[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:30-31
+static bool isValueType(const Type *T) {
+  return !(isa<PointerType>(T) || isa<ReferenceType>(T) || isa<ArrayType>(T) ||
+           isa<MemberPointerType>(T));
 }
----------------
ObjC pointer types?


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:34-41
+static bool isArrayType(QualType QT) { return isa<ArrayType>(QT.getTypePtr()); }
+static bool isReferenceType(QualType QT) {
+  return isa<ReferenceType>(QT.getTypePtr());
+}
+static bool isPointerType(const Type *T) { return isa<PointerType>(T); }
+static bool isPointerType(QualType QT) {
+  return isPointerType(QT.getTypePtr());
----------------
I don't see how these are improvements over checking `QT->isArrayType()` and friends within the caller.


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:82
+    return (llvm::Twine(' ') + DeclSpec::getSpecifierName(Qualifier)).str();
+  return (llvm::Twine(DeclSpec::getSpecifierName(Qualifier)) + llvm::Twine(' '))
+      .str();
----------------
Do you need both casts for `Twine`? I would imagine only the first is needed.


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:166
+
+  llvm_unreachable("All paths should have been handled");
+}
----------------
I think this path is reachable -- it should be an `assert()` instead and return `None`.


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:192
+                                          QualifierPolicy QualPolicy,
+                                          const ASTContext *Context) {
+  assert((QualPolicy == QualifierPolicy::Left ||
----------------
I think `Context` should be by reference instead of by pointer.


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:228-229
+
+  llvm_unreachable(
+      "All possible combinations should have been handled already");
+}
----------------
I think this is reachable as well.


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:27
 
+/// This enum defines where the 'const' shall be preferably added.
+enum class QualifierPolicy {
----------------
const -> qualifier (similar for the comments in the enumerators)


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:33
+
+/// This enum defines which entity is the target for adding the 'const'. This
+/// makes only a difference for pointer-types. Other types behave identical
----------------
Same here.


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:37
+enum class QualifierTarget {
+  Pointee, /// Transforming a pointer goes for the pointee and not the pointer
+           /// itself. For references and normal values this option has no
----------------
goes for -> attaches to?


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:45
+
+/// \brief Creates fix to make ``VarDecl`` const qualified. Only valid if
+/// `Var` is isolated in written code. `int foo = 42;`
----------------
Comment seems out of date here as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395