[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

[don hinton via Phabricator via cfe-commits]

hintonda marked 7 inline comments as done.
hintonda added inline comments.


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:64
+  Finder->addMatcher(
+      parmVarDecl(hasType(pointerType(pointee(parenType(innerType(
+                      functionProtoType(hasDynamicExceptionSpec())))))))
----------------
aaron.ballman wrote:
> Will this catch pointers to member functions as well?
> ```
> struct S {
>   void f() throw();    
> };
> 
> void f(void (S::*)() throw()) {
>     
> }
> ```
Added your test, but will need to change matcher to catch it.


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:68
+      this);
+}
+
----------------
aaron.ballman wrote:
> Also missing: typedefs and using declarations.
As far as I know, it isn't legal to add dynamic exception specifications to typedefs and using declarations.


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:100
+  StringRef ReplacementStr =
+      IsNoThrow ? NoexceptMacro.empty() ? "noexcept" : NoexceptMacro
+                : DtorOrOperatorDel ? "noexcept(false)" : "";
----------------
malcolm.parsons wrote:
> alexfh wrote:
> > Did you consider auto-detection approach like in `getFallthroughAttrSpelling` in tools/clang/lib/Sema/AnalysisBasedWarnings.cpp?
> cpp11-migrate used to do this for -add-override - rL183001.
> clang-tidy's modernize-use-override check doesn't even have an option.
NoexceptMacro is a user option.  I'm just tested whether or not the user provided anything.  Perhaps I should pick a better name.  Would NoexceptMacroOption be better?

Did I misunderstand your comment?


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:105
+  if (IsNoThrow || NoexceptMacro.empty())
+    FixIt = FixItHint::CreateReplacement(CharSourceRange(Range, true),
+                                         ReplacementStr);
----------------
alexfh wrote:
> I suspect this won't work when the range is not contiguous, e.g. starts in a macro definition and ends outside of it:
> 
>   #define T throw
>   void f() T(a, b) {}
> 
> Can you try this test (or construct something similar that will actually break this code)? In case it doesn't work, `Lexer::makeFileCharRange` is the standard way to get a contiguous file range corresponding to a  source range (if possible).
As you suspected, I wasn't handling this case correctly.  I've made the change you suggested and will check in shortly.


================
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:30
+By default this check will only replace ``throw()`` with ``noexcept``,
+and ``throw(<exception>[,...])`` or ``throw(...)`` with blank except
+for operator delete and destructors, which are replaced with
----------------
aaron.ballman wrote:
> I continue to object to removing the explicit exception specification entirely as the default behavior without strong justification. Further. there is no option to control this behavior.
I tried to make sure it's only applied where appropriate.  If I missed a case, please let me know, but I'm not sure an option "just in case" is right solution.

However, I did try to structure the code in a way to make it easy to add an option if that turns out the be the right thing to do.


================
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:38
+that don't support the ``noexcept`` keyword.  Users can define the
+macro to be ``noexcept`` or ``throw()`` depending on whether or not
+noexcept is supported.  Specifications that can throw, e.g.,
----------------
aaron.ballman wrote:
> I'm not certain I understand the justification of calling out older compilers or suggesting use of `throw()`. The check will continually flag `throw()` and try to recommend `noexcept` in its place, won't it? At least, that's how I read the preceding paragraph.
> 
> I think the macro replacement is a reasonable feature, but I think the check only makes sense for C++11 or later, since C++98 users really have no alternatives to dynamic exception specifications.
Libraries, e.g., libc++, that need to support both multiple versions of the standard, use macros to switch between throw() and noexcept.

So this option was designed to be libc++ friendly.  If that's not appropriate, I can remove it.


https://reviews.llvm.org/D20693