[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables
Jonas Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 30 09:00:35 PST 2019
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
More information about the cfe-commits
mailing list