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