JonasToth updated this revision to Diff 167437.
JonasToth added a comment.
- write user-facing documentation
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IsolateDeclCheck.cpp
JonasToth updated this revision to Diff 167400.
JonasToth added a comment.
- last cleanup for today, testing on clang/lib looks good, but is already very
clean
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
JonasToth updated this revision to Diff 167397.
JonasToth added a comment.
- fix doc typo
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IsolateDeclCheck.cpp
JonasToth updated this revision to Diff 167389.
JonasToth added a comment.
- bail out in transformation if there are PP directives in the source range of
the DeclStmt
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
JonasToth marked 9 inline comments as done.
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:211
+ std::vector Snippets;
+ Snippets.reserve(Ranges.size());
+
kbobyrev wrote:
> nit: It would be probably easier to have
JonasToth updated this revision to Diff 167381.
JonasToth marked 9 inline comments as done.
JonasToth added a comment.
- simplification on FIXME:/TODO:
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast(
aaron.ballman wrote:
> JonasToth wrote:
> > kbobyrev
JonasToth updated this revision to Diff 167377.
JonasToth added a comment.
- address review comments, most nits solved
- fix typedefs and function pointers with comments as distraction
- make memberpointer detection more accurate
- move functioning member pointer test
- clean debug output
- clean
JonasToth marked 11 inline comments as done.
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:68
+if (Start.isInvalid() || Start.isMacroID())
+ return SourceLocation();
+ }
kbobyrev wrote:
> Also, I don't think
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast(
JonasToth wrote:
> kbobyrev wrote:
> >
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast(
kbobyrev wrote:
> aaron.ballman wrote:
> > lebedev.ri
JonasToth added a comment.
There are testcases with macro around line 100 in the default tests. I
am not sure yet if `#define I_DECLS int i1, i2, i3;` should be
diagnosed, but the other cases should.
Am 27.09.2018 um 16:28 schrieb Kirill Bobyrev via Phabricator:
> kbobyrev added a comment.
>
>
kbobyrev added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast(
aaron.ballman wrote:
> lebedev.ri wrote:
> > kbobyrev
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast(
lebedev.ri wrote:
> kbobyrev wrote:
> > JonasToth
lebedev.ri added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast(
kbobyrev wrote:
> JonasToth wrote:
> > kbobyrev
kbobyrev added a comment.
Also, regarding error handling and `llvm::Option` vs `llvm::Expected`: I think
the case where the check most likely wouldn't be able to provide useful
diagnostics and perform enough analysis is when there are macro expansions
within inspected statement `SourceRange`.
kbobyrev added a comment.
A lot of good improvements and many test cases, thank you!
The comments are mostly nits.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+
JonasToth updated this revision to Diff 167282.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.
- add space after clang-tidy in banner
- refactor slightly, add better in-code comments to explain whats happening,
add more TODOs and FIXMEs
Repository:
rCTE Clang Tools
JonasToth updated this revision to Diff 167270.
JonasToth marked 6 inline comments as done.
JonasToth added a comment.
- move lexer stuff into LexerUtils
- remove custom string handling function
- address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:21
+
+#define PRINT_DEBUG 1
+
kbobyrev wrote:
> Do you plan to submit the patch with debugging messages or are you just using
> these for better local debugging experience?
kbobyrev added a comment.
Sorry, I didn't get time to review the patch properly, these are few stylistic
comments. Hopefully, I'll be able to give more feedback when I get more time.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:21
+
+#define PRINT_DEBUG 1
+
JonasToth updated this revision to Diff 166044.
JonasToth added a comment.
- minor adjustments, add fixmes
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IsolateDeclCheck.cpp
JonasToth updated this revision to Diff 166041.
JonasToth added a comment.
- Merge branch 'master' into experiment_isolate_decl
- further work on isolate decl, fix tokenizing bug with templates
- include big test-suite and make them run
Repository:
rCTE Clang Tools Extra
JonasToth updated this revision to Diff 165752.
JonasToth marked an inline comment as done.
JonasToth added a comment.
- use static functions instead anonymous namespace
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
JonasToth marked 5 inline comments as done.
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:71
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "make only one declaration per
statement");
+
kbobyrev wrote:
> Maybe
JonasToth updated this revision to Diff 165749.
JonasToth marked an inline comment as done.
JonasToth added a comment.
- improve diagnostic to include number of declared variables
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
JonasToth marked 7 inline comments as done.
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:51
+
+return TypeAndName + " = " + Initializer + ";";
+ }
kbobyrev wrote:
> JonasToth wrote:
> > kbobyrev wrote:
> > > This
JonasToth updated this revision to Diff 165742.
JonasToth added a comment.
- [Feature] initial commit for extract decl experiment
- [Feature] implement the last-element extraction version kinda ok
- [Misc] work further on the experiment
- do a full transformation of the multi decl stmt
- [Misc]
lebedev.ri added a comment.
In https://reviews.llvm.org/D51949#1233951, @JonasToth wrote:
> Yes, do you think it should be included in the diag?
Yes, please :) Else, the message seems a bit too empty.
I **don't** think it should point (via `NOTE:`) at the each decl though.
> Am 13.09.2018 um
JonasToth added a comment.
Yes, do you think it should be included in the diag?
Am 13.09.2018 um 22:09 schrieb Roman Lebedev via Phabricator:
> lebedev.ri added inline comments.
>
>
> Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:200
> +
> +
lebedev.ri added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:200
+
+ diag(WholeDecl->getBeginLoc(), "make only one declaration per statement")
+ << FixItHint::CreateReplacement(WholeDecl->getSourceRange(),
Replacement);
I
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:35
+namespace {
+SourceLocation findNextTerminator(SourceLocation Start, const SourceManager
,
+ const LangOptions ) {
Remove duplication,
JonasToth updated this revision to Diff 165321.
JonasToth added a comment.
This is the newer and better working code to slice and separate multiple
vardecls in a multi-decl stmt.
I want this version only for now, as the multiple variable definitions in one
statement are by far the most common
JonasToth added a comment.
In https://reviews.llvm.org/D51949#1232443, @kbobyrev wrote:
> I've been thinking about corner-cases (e.g. don't split `DeclStmt`s within
> init-statement declaration) a while and it seems that there might be many of
> them.
Yes, things I am currently thinnking of:
lebedev.ri added a comment.
In https://reviews.llvm.org/D51949#1232443, @kbobyrev wrote:
> I've been thinking about corner-cases (e.g. don't split `DeclStmt`s within
> init-statement declaration) a while and it seems that there might be many of
> them.
It should at least diagnose these
kbobyrev added a comment.
I've been thinking about corner-cases (e.g. don't split `DeclStmt`s within
init-statement declaration) a while and it seems that there might be many of
them.
I didn't notice https://reviews.llvm.org/D27621 in the first place, it seems to
have a solid test suite (and
JonasToth added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:35
+
+ std::string TypeAndName =
+ VarType.getAsString(TypePrinter) + " " + D->getNameAsString();
kbobyrev wrote:
> `llvm::Twine` here? Seems like this one is used a
kbobyrev added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:35
+
+ std::string TypeAndName =
+ VarType.getAsString(TypePrinter) + " " + D->getNameAsString();
`llvm::Twine` here? Seems like this one is used a lot for
JonasToth added a comment.
I knew there was something already but I couldn't find it. Thank you for
pointing me there :)
I will inspect the code and then decide, maybe I will incorporate some
of his stuff into mine or take his one over.
Am 12.09.2018 um 08:04 schrieb Roman Lebedev via
lebedev.ri added a comment.
Thank you for working on this! I really miss this check.
To be noted, there is preexisting, almost finished version -
https://reviews.llvm.org/D27621.
I'm not sure in what state it really is, but it might be simpler to take that
over and finish it.
Repository:
JonasToth added a comment.
Ty for the initial review, I am currently rewriting the approach for it.
I will probably make the business logic as standalone class/functions to
utilize this functionality in multiple checks.
I first try to evaluate my approach and as I had many unclear things i
Eugene.Zelenko added a comment.
I would suggest to use declaration or declarations in check name instead of
abbreviation.
Actually there are existing coding guidelines for thus rule, like SEI CERT
Eugene.Zelenko added inline comments.
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:29
+
+namespace {
+std::string GetIsolatedDecl(const VarDecl *D, const SourceManager ,
Please use static instead.
Comment at:
JonasToth created this revision.
JonasToth added reviewers: rsmith, aaron.ballman, alexfh, hokein.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
The idea of this check is to enforce one decl per statement and to include
an automated code transformation for all multi-decl statments.
It
44 matches
Mail list logo
