[PATCH] D18575: [clang-tidy] New checker to replace deprecated throw() specifications

[don hinton via cfe-commits]

hintonda added inline comments.

================
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.
----------------
aaron.ballman wrote:
> hintonda wrote:
> > aaron.ballman wrote:
> > > Pathologically terrible counter-case: `void func() throw(decltype(throw 12) *)`
> > Good point, looks like I need a full fledged parser to catch 100% or the cases -- or we could ignore these corner cases.
> At the very least, we should have test cases showing what the behavior is with a big FIXME around this code, should you decide to keep it. I'm not keen on the idea of this being part of a fixit that may destroy well-defined user code. Same for the assumptions about the location of right parens. That code looks equally broken even without multiple `throw` tokens in the stream. Consider:
> `void func() throw(int(int));`
Had thought about adding paren parsing, but wasn't sure it was needed -- thanks for pointing out that it is.  Of course, I'll have to parse from the beginning to do this correctly, but that's not a big deal.

================
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))
----------------
aaron.ballman wrote:
> Can we use `>= 0` instead of `!= -1`? It makes it more immediately obvious that the array index will not underflow.
Sure.

================
Comment at: test/clang-tidy/modernize-use-noexcept-macro.cpp:11
@@ +10,3 @@
+
+// Should not trigger a FixItHint
+class A {};
----------------
aaron.ballman wrote:
> I may have missed some context in the discussion, but why shouldn't this trigger a FixItHint?
Because the user provided a macro.  The only reason you would do that is if you want the macro to expand to something different depending on whether or not 'noexcept' is supported.  

Perhaps this can be done more elegantly, but the use case for this entire checker was libcxx.  It defines _NOEXCEPT as either noexcept or throw() depending on whether or not noexcept is supported.  I don't see a good way of doing that, other than removing it completely, so I just reported it without supplying a FixItHint.



http://reviews.llvm.org/D18575