This revision was automatically updated to reflect the committed changes.
Closed by commit rL316662: [Sema] -Wzero-as-null-pointer-constant: don't warn
for system macros other than… (authored by lebedevri).
Changed prior to commit:
https://reviews.llvm.org/D38954?vs=120316=120402#toc
thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.
In general, "I don't see any way to prevent $slow_thing" just means the patch
can't move ahead. But in this case, this looks like fairly small overhead for a
worst-case input, so this won't
lebedev.ri updated this revision to Diff 120316.
lebedev.ri added a comment.
Add `Diags.isIgnored()` early return + move `C++11` early return earlier too.
At least partially should help avoid the penalty of `findMacroSpelling()`
I don't think i see any way to avoid `findMacroSpelling()` :/
Hm, that's a lot of overhead. Granted, it's a synthetic benchmark, but I
think it'd be good to try this on some real codebase to make sure it really
is negligible overhead in real-world scenarios.
On Wed, Oct 25, 2017 at 3:09 PM, Roman Lebedev via Phabricator via
cfe-commits
lebedev.ri updated this revision to Diff 120305.
lebedev.ri added a comment.
Rebased now that https://reviews.llvm.org/D39301 has landed.
Repository:
rL LLVM
https://reviews.llvm.org/D38954
Files:
docs/ReleaseNotes.rst
lib/Sema/Sema.cpp
test/SemaCXX/Inputs/warn-zero-nullptr.h
lebedev.ri added a comment.
In https://reviews.llvm.org/D38954#906900, @thakis wrote:
> Can you build some large-ish codebase (say, LLVM) with and without this patch
> and make sure that this doesn't measurably add to build perf? (With the
> warning turned on, obviously.)
>
> Other than that,
thakis added a comment.
Can you build some large-ish codebase (say, LLVM) with and without this patch
and make sure that this doesn't measurably add to build perf? (With the warning
turned on, obviously.)
Other than that, this looks good to me.
Comment at:
lebedev.ri updated this revision to Diff 120285.
lebedev.ri added a comment.
- Rebased
- Handle `-Wsystem-headers` properly, as per irc disscussion.
It was suggested that `findMacroSpelling()` might be slow, and
`isNullPointerConstant()` should be used, but i'm not sure any of the
lebedev.ri added inline comments.
Comment at: lib/Sema/Sema.cpp:445
+ // If it is a macro from system header, and if the macro name is not "NULL",
+ // do not warn.
+ SourceLocation MaybeMacroLoc = E->getLocStart();
Rakete wrote:
> That comment doesn't
lebedev.ri updated this revision to Diff 119892.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Use `MaybeMacroLoc` variable in the other place too.
Repository:
rL LLVM
https://reviews.llvm.org/D38954
Files:
docs/ReleaseNotes.rst
lib/Sema/Sema.cpp
Rakete added inline comments.
Comment at: lib/Sema/Sema.cpp:445
+ // If it is a macro from system header, and if the macro name is not "NULL",
+ // do not warn.
+ SourceLocation MaybeMacroLoc = E->getLocStart();
That comment doesn't really add anything
lebedev.ri updated this revision to Diff 119833.
lebedev.ri retitled this revision from "[Sema] -Wzero-as-null-pointer-constant:
don't warn for system macros." to "[Sema] -Wzero-as-null-pointer-constant:
don't warn for system macros other than NULL.".
lebedev.ri edited the summary of this
lebedev.ri updated this revision to Diff 119159.
lebedev.ri added a comment.
Address @thakis review notes: do make sure that we still warn on `NULL`.
Any other special macros/cases?
Repository:
rL LLVM
https://reviews.llvm.org/D38954
Files:
lib/Sema/Sema.cpp
lebedev.ri added a comment.
In https://reviews.llvm.org/D38954#898586, @thakis wrote:
> As said on the bug, this matches gcc's behavior,
https://bugs.llvm.org/show_bug.cgi?id=33771#c3
> and with this you won't see this warning for NULL.
Finally, an argument that can actually be addressed.
>
thakis added a comment.
As said on the bug, this matches gcc's behavior, and with this you won't see
this warning for NULL. I don't think this is better.
Repository:
rL LLVM
https://reviews.llvm.org/D38954
___
cfe-commits mailing list
lebedev.ri created this revision.
lebedev.ri added a project: clang.
The warning was initially introduced in https://reviews.llvm.org/D32914 by
@thakis,
and the concerns were raised there, and later in
https://reviews.llvm.org/rL302247
and PR33771.
I do believe that it makes sense to relax the
16 matches
Mail list logo
