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