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

[don hinton via Phabricator via cfe-commits]

hintonda added inline comments.


================
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:68
+      this);
+}
+
----------------
aaron.ballman wrote:
> hintonda wrote:
> > 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.
> Hmm, I thought you could, at least in C++17, since it's now part of the function's type. However, I don't have a standard in front of me at the moment, so I'll have to double-check. It can always be added in a follow-up patch and need not block this one.
> 
> However, I do know the following is allowed in a typedef, and I don't think your existing ParmVarDecl matcher will catch it:
> ```
> typedef void (*fp)(void (*f)(int) throw());
> ```
clang doesn't allow dynamic exception specs in typedef or using declarations for either c++11 or c++14, but does in c++1z since throw() is an alias for noexcept.  I'll try to add an appropriate test to highlight this.


I've added your typedef example -- thanks for suggesting it -- and the matcher does catch it and does the appropriate fixit.


================
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:
> hintonda wrote:
> > 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.
> The behavior as it's implemented now is not the behavior I would expect from such a check -- I think that `throw(int)` should get a FixIt to `noexcept(false)` rather than no `noexcept` clause whatsoever. Despite them being functionally equivalent in most cases, the explicit nature of the dynamic exception specification should be retained with an explicit noexcept exception specification, not discarded. If you really want `throw(int)` to be modified to have no explicit exception specification, I think that should be an option (off by default). If you would rather not make an option, then I think the explicit exception specification should remain.
Makes sense.  Will 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:
> hintonda wrote:
> > 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.
> I think the *option* is fine; I think the wording is confusing. How about:
> ```
> Alternatively, users can also use :option:`ReplacementString` to
> specify a macro to use instead of ``noexcept``.  This is useful when
> maintaining source code that uses custom exception specification marking
> other than ``noexcept``.
> ```
Ah, okay, that sounds much better.  I'll make this change shortly.


https://reviews.llvm.org/D20693