aaron.ballman added inline comments.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:21
@@ +20,3 @@
+
+static StringRef
+makeDynamicExceptionString(const SourceManager &SM,
----------------
Instead of a bunch of static functions, would an unnamed namespace make more
sense?
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:34
@@ +33,3 @@
+
+static CharSourceRange makeMoveRange(const SourceManager &SM,
+ const LangOptions &LangOps,
----------------
Since this function is only called one time and is a one-liner, perhaps it
makes more sense to inline the body at the call site?
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:46
@@ +45,3 @@
+ const SmallVector<Token, 16> &Tokens) {
+ // Find throw token -- it's a keyword, so there can't be more than one.
Also,
+ // it should be near the end of the declaration, so search from the end.
----------------
Pathologically terrible counter-case: `void func() throw(decltype(throw 12) *)`
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:49
@@ +48,3 @@
+ int TokenIndex;
+ for (TokenIndex = Tokens.size() - 1; TokenIndex != -1; --TokenIndex) {
+ if (Tokens[TokenIndex].is(tok::kw_throw))
----------------
Can we use `>= 0` instead of `!= -1`? It makes it more immediately obvious that
the array index will not underflow.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:100
@@ +99,3 @@
+
+ auto FileMoveRange = createReplacementRange(SM, getLangOpts(), Tokens);
+
----------------
Please don't use `auto` here; I have no idea what type `FileMoveRange` will
have from inspecting the code.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:108
@@ +107,3 @@
+
+ diag(FuncDecl->getLocation(), "function '%0' uses dynamic exception "
+ "specification '%1'")
----------------
No need to quote %0, the diagnostics engine will already do it when passed a
NamedDecl, so this will improperly double-quote the diagnostic.
================
Comment at: clang-tidy/modernize/UseNoexceptCheck.h:22
@@ +21,3 @@
+/// \brief Replace dynamic exception specifications, with
+/// `noexcept` (or user-defined macro) or `noexcept(true)`.
+/// \code
----------------
I think this comment means `noexcept(false)` instead? Or is there a reason to
replace with `noexcept(true)` instead of just `noexcept`?
================
Comment at: test/clang-tidy/modernize-use-noexcept-macro.cpp:11
@@ +10,3 @@
+
+// Should not trigger a FixItHint
+class A {};
----------------
I may have missed some context in the discussion, but why shouldn't this
trigger a FixItHint?
http://reviews.llvm.org/D18575
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
