This revision was automatically updated to reflect the committed changes.
Closed by commit rL304977: [clang-tidy] New checker to replace dynamic
exception specifications (authored by alexfh).
Changed prior to commit:
https://reviews.llvm.org/D20693?vs=101860=101910#toc
Repository:
rL LLVM
alexfh added a comment.
In https://reviews.llvm.org/D20693#776030, @hintonda wrote:
> Great, thanks for you help.
>
> Could you commit this for me?
Sure, running tests...
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
hintonda added a comment.
Great, thanks for you help.
Could you commit this for me?
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
Now that you found the root cause, this looks like a proper fix ;)
Thank you! LG
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
hintonda updated this revision to Diff 101860.
hintonda added a comment.
- Make sure types for ternary operator are the same.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseNoexceptCheck.cpp
hintonda added a comment.
Here's a simple example that demonstrates the corruption I'm seeing:
#include "llvm/ADT/StringRef.h"
int main() {
std::string ss = "";
llvm::StringRef Ref = true ? "noexcept" : ss;
std::string s = Ref;
return 0;
}
https://reviews.llvm.org/D20693
hintonda added a comment.
btw, here's how I built it, in case that matters...
CC=../../4.0.0/build/Release/bin/clang
CXX=../../4.0.0/build/Release/bin/clang++ \
cmake ../../llvm/ \
-GNinja \
-DLLVM_USE_SANITIZER=Address \
-DCMAKE_BUILD_TYPE=Debug \
-DLLVM_TARGETS_TO_BUILD="X86" \
hintonda added a comment.
Just ran asan on linux and we have a heap-use-after-free in the std::string
ctor.
Here's a partial stack dump:
4980==ERROR: AddressSanitizer: heap-use-after-free on address 0x60424328 at
pc 0x0057ad32 bp 0x7ffd240a7f50 sp 0x7ffd240a7700
hintonda updated this revision to Diff 101819.
hintonda added a comment.
- Rollback last change.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseNoexceptCheck.cpp
alexfh added a comment.
In https://reviews.llvm.org/D20693#775197, @hintonda wrote:
> I have not, as yet, been able to reproduce the buildbot failures. They were
> essentially intermittent seg-faults, and corrupt diag output.
>
> I will work on creating a test that can reproduce the problem.
hintonda added a comment.
I have not, as yet, been able to reproduce the buildbot failures. They were
essentially intermittent seg-faults, and corrupt diag output.
I will work on creating a test that can reproduce the problem.
https://reviews.llvm.org/D20693
alexfh added a comment.
In https://reviews.llvm.org/D20693#775153, @alexfh wrote:
> In https://reviews.llvm.org/D20693#775014, @hintonda wrote:
>
> > - Only pass %2 parameter if %2 is included in format.
>
>
> I thought, DiagnosticsBuilder handles placeholders in conditional parts
> correctly.
alexfh added a comment.
In https://reviews.llvm.org/D20693#775014, @hintonda wrote:
> - Only pass %2 parameter if %2 is included in format.
I thought, DiagnosticsBuilder handles placeholders in conditional parts
correctly. Did you find an evidence of the opposite? Can you add a test that
hintonda updated this revision to Diff 101722.
hintonda added a comment.
- Only pass %2 parameter if %2 is included in format.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
hintonda added a comment.
Unfortunately, the logs are no longer available, and I don't have copies.
However, I think I know what's going on, so I'll try to submit a fix later
today.
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
hintonda updated this revision to Diff 101708.
hintonda added a comment.
- Rollback to previous version: rebased + r293218 and r293234.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
In any case, I'm strongly against these hacks, please revert them.
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
alexfh added a comment.
In https://reviews.llvm.org/D20693#774693, @hintonda wrote:
> In order to fix diagnostic corruption in some of the buildbot tests
> (unable to reproduce locally):
>
> - make NoexceptMacro a static variable so it's lifetime doesn't end when
> UseNoexceptCheck is
hintonda updated this revision to Diff 101659.
hintonda added a comment.
Herald added a subscriber: xazax.hun.
In order to fix diagnostic corruption in some of the buildbot tests
(unable to reproduce locally):
- make NoexceptMacro a static variable so it's lifetime doesn't end when
hintonda reopened this revision.
hintonda added a comment.
This revision is now accepted and ready to land.
Reopening due to test failures on Linux -- was rolled back.
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
hintonda added a comment.
Thanks and sorry for the breakage. Unfortunately, I'm unable to reproduce
locally (OSX), but will try to get access to linux box this weekend.
Seems to be related to memory corruption wrt to improper StringRef usage, but I
can't say for sure yet.
aaron.ballman added a comment.
There were some issues with failing tests that caused this commit to need to be
reverted in r293267
See for instance:
http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/1039
Eugene Zelenko also had some small fixes you might want to incorporate as
aaron.ballman added a comment.
In https://reviews.llvm.org/D20693#658087, @hintonda wrote:
> Great, thanks. Could you commit for me?
Certainly! I've commit in r293217.
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
hintonda added a comment.
Great, thanks. Could you commit for me?
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Yes, this LGTM as well. Thank you for working on this!
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
alexfh accepted this revision.
alexfh added a comment.
LGTM, if Aaron has no concerns.
Thank you for the new check!
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
hintonda updated this revision to Diff 85593.
hintonda marked 4 inline comments as done.
hintonda added a comment.
- Addressed comments and added additional test cases.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:20-33
+static StringRef
+makeDynamicExceptionString(const SourceManager ,
+
hintonda updated this revision to Diff 85291.
hintonda added a comment.
- Rebased.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseNoexceptCheck.cpp
clang-tidy/modernize/UseNoexceptCheck.h
hintonda updated this revision to Diff 84975.
hintonda added a comment.
- Fix diagnostic when removing throwing specs.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseNoexceptCheck.cpp
hintonda added a comment.
Thanks for all the feedback. I've tried to address all your comments, and
reworked the documentation. Please let me know if I missed or misunderstood
anything.
https://reviews.llvm.org/D20693
___
cfe-commits mailing
hintonda updated this revision to Diff 84926.
hintonda marked 22 inline comments as done.
hintonda added a comment.
- Add support for matching MemberPointerType's.
- Add noexcept(false) option plus test; allow invalid replacement ranges;
enhance diagnostics.
- Updated docs.
aaron.ballman added inline comments.
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:6-8
+The check converts dynamic exception specifications, e.g.,
+``throw()``, ``throw([,...])``, or ``throw(...)``, to
+``noexcept``, ``noexcept(false)``, blank, or a user defined
hintonda added inline comments.
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:6-8
+The check converts dynamic exception specifications, e.g.,
+``throw()``, ``throw([,...])``, or ``throw(...)``, to
+``noexcept``, ``noexcept(false)``, blank, or a user defined
alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:99
+
+ assert(CRange.isValid() && "Exception Specification Range is invalid.");
+ assert(FnTy &&
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
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:68
+ this);
+}
+
hintonda wrote:
> aaron.ballman wrote:
> > Also missing: typedefs and using declarations.
> As far as I know, it isn't legal to add dynamic exception
hintonda updated this revision to Diff 83827.
hintonda marked an inline comment as done.
hintonda added a comment.
- Addressed comments.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
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(
+
aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:41
+void UseNoexceptCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+return;
malcolm.parsons added inline comments.
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:100
+ StringRef ReplacementStr =
+ IsNoThrow ? NoexceptMacro.empty() ? "noexcept" : NoexceptMacro
+: DtorOrOperatorDel ? "noexcept(false)" : "";
alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:100
+ StringRef ReplacementStr =
+ IsNoThrow ? NoexceptMacro.empty() ? "noexcept" : NoexceptMacro
+
hintonda updated this revision to Diff 83588.
hintonda added a comment.
- Omit noexcept(false) replacement except for dtor and operator delete.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
aaron.ballman added a comment.
In https://reviews.llvm.org/D20693#639030, @hintonda wrote:
> Matthias, I think you make a good point. While noexcept(expr) is valuable,
> noexcept(false) adds no value. Therefore, I think we should do as you
> suggest, i.e.:
>
> s/throw()/noexcept/
>
hintonda added a comment.
Matthias, I think you make a good point. While noexcept(expr) is valuable,
noexcept(false) adds no value. Therefore, I think we should do as you suggest,
i.e.:
s/throw()/noexcept/
s/throw(something)//
Aaron, does this work for you?
mgehre added a comment.
I'm working on code bases where their is no manually written noexcept(false)
anywhere,
and I don't think the FixIt should look different than manually written code.
At least a configuration option for the check would be nice.
https://reviews.llvm.org/D20693
aaron.ballman added a comment.
In https://reviews.llvm.org/D20693#638062, @mgehre wrote:
> Hi,
> this is a good idea for a check.
>
> I would prefer when the FixIt just removes throw(A,B) specifier, instead of
> replacing it by noexcept(false),
> because noexcept(false) means the same things
hintonda added a comment.
I could be wrong (please let me know if I am), but my understanding is:
// Does not throw
throw() --> noexcept == noexcept(true)
// Does throw
throw(something) --> noexcept(false)
Please see http://en.cppreference.com/w/cpp/language/noexcept_spec
mgehre added a comment.
Hi,
this is a good idea for a check.
I would prefer when the FixIt just removes throw(A,B) specifier, instead of
replacing it by noexcept(false),
because noexcept(false) means the same things as having no noexcept specifier
at all.
And less code to read means its easier
hintonda updated this revision to Diff 83375.
hintonda added a comment.
- Addressed comments.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseNoexceptCheck.cpp
hintonda added inline comments.
Comment at: clang-tidy/modernize/UseNoexceptCheck.h:19
+
+using CandidateSet = llvm::StringSet;
+
malcolm.parsons wrote:
> Unused?
Good catch -- left over from a previous version.
Comment at:
malcolm.parsons added inline comments.
Comment at: clang-tidy/modernize/UseNoexceptCheck.h:19
+
+using CandidateSet = llvm::StringSet;
+
Unused?
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:38
+
+Please note that since
hintonda updated this revision to Diff 83371.
hintonda added a comment.
Herald added subscribers: JDevlieghere, mgorny.
Updated patch.
https://reviews.llvm.org/D20693
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
alexfh added a comment.
Removing from my dashboard until http://reviews.llvm.org/D20428 is submitted.
http://reviews.llvm.org/D20693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
Removing from my dashboard until http://reviews.llvm.org/D20428 is submitted.
http://reviews.llvm.org/D20693
___
cfe-commits mailing
55 matches
Mail list logo
