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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
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());
----------------
aaron.ballman wrote:
> I don't see how these are improvements over checking `QT->isArrayType()` and friends within the caller.
True!


================
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();
----------------
aaron.ballman wrote:
> Do you need both casts for `Twine`? I would imagine only the first is needed.
In case of `' '` it is necessary, in case of `" "` its not. I go with `" "`, should not be more expensive or anything, right?


================
Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:166
+
+  llvm_unreachable("All paths should have been handled");
+}
----------------
aaron.ballman wrote:
> I think this path is reachable -- it should be an `assert()` instead and return `None`.
When testing LLVM i saw some path triggering the assertions and unreachables.

I want to go the `None`-route for now and only emit warnings without fixit. That will help to find false-positives unhandled cases better and is less problematic.

Is it only objc-code that could trigger this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395