aaron.ballman added inline comments.

================
Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:55-71
+  if (const auto *Reference =
+          Result.Nodes.getNodeAs<VarDecl>("reference_to_non-const")) {
+    diag(Reference->getLocation(),
+         "variable %0 provides global access to non-const type, consider "
+         "making the referenced data const")
+        << Reference; // FIXME: Add fix-it hint to Reference
+    return;
----------------
I think these cases should be combined to avoid duplication.
```
if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("whatever")) {
  diag(VD->getLocation(), "variable %0 provides global access to a non-const 
object; considering making the %select{referenced|pointed-to}1 data 'const'") 
<< VD << VD->getType()->isReferenceType();
  return;
}
```
the matcher needs to be changed to bind to the same id for both cases.

Note, this rewords the diagnostic slightly to avoid type/object confusion (the 
variable provides access to an object of a non-const type, not to the type 
itself).


================
Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:20
+
+  char * c_ptr1 = &c;  // Warns!
+  char *const c_const_ptr = &c;  // Warns!
----------------
This isn't legal code.


================
Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst:33
+    char h = 0;
+  }
+
----------------
This isn't legal code either.

You may want to run the example through the compiler to catch these sort of 
things.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70265/new/

https://reviews.llvm.org/D70265



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to