This revision was automatically updated to reflect the committed changes.
Closed by commit rC347339: [clang][Parse] Diagnose useless null statements /
empty init-statements (authored by lebedevri, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D52695?vs=174624&id=174812#toc
lebedev.ri added a comment.
In https://reviews.llvm.org/D52695#1304225, @aaron.ballman wrote:
> Aside from some small nits, LGTM!
Thank you for the review!
Will address nits and land.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cf
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Aside from some small nits, LGTM!
Comment at: docs/ReleaseNotes.rst:53
+- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like
+ ``-Wextra-semi``,
lebedev.ri updated this revision to Diff 174624.
lebedev.ri marked 12 inline comments as done.
lebedev.ri added a comment.
Address @aaron.ballman review notes.
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
incl
lebedev.ri added inline comments.
Comment at: test/Parser/extra-semi-resulting-in-nullstmt.cpp:81
+ }
+}
aaron.ballman wrote:
> There are four additional tests I'd like to see: 1) a semicolon at global
> scope, 2) a semicolon after a namespace declaration (e.g.
aaron.ballman added a comment.
Sorry about the delayed review on this -- I had intended to provide comments
earlier, but somehow this fell off my radar.
Comment at: docs/ReleaseNotes.rst:53
+- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like
+ ``-Wextra-semi``
lebedev.ri added a comment.
Ping. Is even the `-Wempty-init-stmt` is not interesting?
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
lebedev.ri added a comment.
ping.
I wonder if @aaron.ballman has any opinion on this.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
lebedev.ri added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
ping
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri updated this revision to Diff 168240.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Thank you for taking a look!
- Reworded the diag as suggested a little
- Do consume consequtive semis as one
- Do still record each one of them in AST. I'm not sure if we wa
rsmith added a comment.
I feel indifferent about this warning, but clearly you care about it enough to
work on it, and it's a pretty low-cost feature from a maintenance perspective,
so I suppose it's fine.
Comment at: include/clang/Basic/DiagnosticParseKinds.td:56-58
+def war
lebedev.ri updated this revision to Diff 168157.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.
After looking at stage-2 of LLVM, remove `-Wextra-semi-stmt` from `-Wextra`,
and add `-Wempty-init-stmt` back into `-Wextra`.
Anything else?
Repository:
rC Clang
https://
lebedev.ri added inline comments.
Comment at: include/clang/Basic/DiagnosticGroups.td:770
+NullPointerArithmetic,
+ExtraSemiStmt
]>;
I'm really unsure of this. Maybe this should only be `EmptyInitStatement`.
Repository:
rC Clang
https://reviews.ll
lebedev.ri updated this revision to Diff 168020.
lebedev.ri added a comment.
Slightly improved test coverage for macros in
`extra-semi-resulting-in-nullstmt-in-init-statement.cpp`
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/Diagn
lebedev.ri added inline comments.
Comment at: include/clang/Basic/DiagnosticGroups.td:167-168
+ CXX11ExtraSemi,
+ ExtraSemiStmt,
+ EmptyInitStatement]>;
-
lebedev.ri updated this revision to Diff 168017.
lebedev.ri marked 3 inline comments as done.
lebedev.ri added a comment.
- Moved `-Wempty-init-stmt` into `-Wextra-semi-stmt`
- Moved `-Wextra-semi-stmt` out of `-Wextra-semi`
- Tentatively enabled `-Wextra-semi-stmt` in `-Wextra` (and removed
`-We
rsmith added inline comments.
Comment at: include/clang/Basic/DiagnosticGroups.td:164
+def ExtraSemiStmt : DiagGroup<"extra-semi-stmt">;
+def EmptyInitStatement : DiagGroup<"empty-init-stmt">;
def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
I thin
lebedev.ri added inline comments.
Comment at: lib/Parse/ParseStmt.cpp:237
+SourceLocation SemiLocation = ConsumeToken();
+if (!HasLeadingEmptyMacro && getCurScope()->isCompoundStmtScope() &&
+!SemiLocation.isMacroID()) {
rsmith wrote:
> I'm a litt
lebedev.ri updated this revision to Diff 168000.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.
Thank you for taking a look!
- Move it into `ParseCompoundStatementBody()`, thus fixing false-positives with
`case X: ;` e.g.
- Rename to `-Wextra-semi-stmt`
- Add `-Wempty-i
rsmith added a comment.
Like Eli, I don't see much value in this warning. But it doesn't seem much
different from `-Wextra-semi` in that regard for languages in which such extra
semicolons are generally valid -- I expect both warnings will generally
diagnose typos and little else.
Does this wa
efriedma added a comment.
Again, I'm not sure we really want this... we don't really like adding new
off-by-default warnings, and we don't usually add diagnostics to enforce style.
Comment at: include/clang/Basic/DiagnosticGroups.td:163
def CXX11ExtraSemi : DiagGroup<"c++11-e
lebedev.ri created this revision.
lebedev.ri added reviewers: rsmith, aaron.ballman, efriedma.
clang has `-Wextra-semi` (https://reviews.llvm.org/D43162), which is not
dictated by the currently selected standard.
While that is great, there is at least one more source of need-less semis -
'null s
25 matches
Mail list logo
