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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
