JDevlieghere updated this revision to Diff 88550.
JDevlieghere added a comment.
Fixed latest comment from @aaron.ballman before landing.
Repository:
rL LLVM
https://reviews.llvm.org/D28768
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
aaron.ballman accepted this revision.
aaron.ballman added a comment.
There's one small wording nit with the diagnostic, but once that's resolved,
LGTM as well.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:65
+
+ auto Diag = diag(Loc, "to avoid repeating the
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
LGTM. Please wait for Aaron as well.
Repository:
rL LLVM
https://reviews.llvm.org/D28768
___
cfe-commits mailing list
JDevlieghere updated this revision to Diff 88409.
JDevlieghere marked 4 inline comments as done.
JDevlieghere added a comment.
- Updated the diff with comments from @alexfh
Thanks for mentioning the check fixes pattern, it's quite crucial but I never
really thought about that!
Repository:
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
A few more nits.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:66
+ auto Diag = diag(Loc, "to avoid repeating the return type from the "
+
JDevlieghere updated this revision to Diff 88226.
JDevlieghere marked 5 inline comments as done.
JDevlieghere added a comment.
Thanks for reviewing @aaron.ballman and @alexfh. I have updated the diff to
address your issues. While looking at the logic that checked for matching
argument types I
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
A couple of comments.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:35
+
+ auto HasConstructExpr = has(ConstructExpr);
+
Nit: I
aaron.ballman added a comment.
I found a few more small nits, but basically LGTM.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:70
+
+ for (int I = 0, NumParams = MatchedFunctionDecl->getNumParams();
+ I < NumParams; ++I) {
`I` should
JDevlieghere updated this revision to Diff 87823.
JDevlieghere marked 11 inline comments as done.
JDevlieghere added a comment.
- Fixed issues raised by @alexfh
Repository:
rL LLVM
https://reviews.llvm.org/D28768
Files:
clang-tidy/modernize/CMakeLists.txt
alexfh added a comment.
Please mark all addressed comments "Done".
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:60
+ // Make sure that the return type matches the constructed type.
+ const QualType returnType =
+
JDevlieghere updated this revision to Diff 87228.
JDevlieghere added a comment.
- Add explicit check to matcher as suggested by @aaron.ballman
Repository:
rL LLVM
https://reviews.llvm.org/D28768
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:59
+ // Skip explicit construcotrs.
+ if (MatchedConstructExpr->getConstructor()->isExplicit())
+return;
Can't this be handled as part of the matcher?
JDevlieghere updated this revision to Diff 85937.
JDevlieghere added a comment.
- Added missing instantiations in test
Repository:
rL LLVM
https://reviews.llvm.org/D28768
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: test/clang-tidy/modernize-return-braced-init-list.cpp:150
+template
+T f16() {
+ return T();
"With multiple instantiations" is
JDevlieghere updated this revision to Diff 84700.
JDevlieghere added a comment.
- Added test cases suggested by @Prazek
- Added test cases suggested by @alexfh
I don't match on `CXXUnresolvedConstructExpr` so the template dependent cases
are not impacted by this check. This is what I intended,
malcolm.parsons added a comment.
This fixes PR24967.
Repository:
rL LLVM
https://reviews.llvm.org/D28768
___
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 inline comments.
This revision now requires changes to proceed.
Comment at: test/clang-tidy/modernize-return-braced-init-list.cpp:132
+auto v1 = []() { return vector({1, 2}); }();
+auto v2 = []() -> vector { return
JDevlieghere added a comment.
In https://reviews.llvm.org/D28768#647949, @Prazek wrote:
> Do you have some results from running it on LLVM? If nothing crashes and all
> fixit are correct then LGTM.
There's still an issue with type narrowing, which is allowed in regular
constructor but not
Prazek accepted this revision.
Prazek added a comment.
This revision is now accepted and ready to land.
Do you have some results from running it on LLVM? If nothing crashes and all
fixit are correct then LGTM.
Comment at:
JDevlieghere updated this revision to Diff 84589.
JDevlieghere added a comment.
- Don't replace explicit constructors
- Add @Prazek's suggested tests
Repository:
rL LLVM
https://reviews.llvm.org/D28768
Files:
clang-tidy/modernize/CMakeLists.txt
Prazek added inline comments.
Comment at: test/clang-tidy/modernize-return-braced-init-list.cpp:95
+}
+
+vector f6() {
please also add test that contains
for function
Type foo():
return call(Type());
return OtherType(Type()); // implicit conversion
It would
JDevlieghere added a comment.
In https://reviews.llvm.org/D28768#647333, @Prazek wrote:
> Thanks for the check. Have you run it on llvm?
Not yet, there was an issue with templated types and I wanted to fix that
first. It's running now.
Repository:
rL LLVM
https://reviews.llvm.org/D28768
JDevlieghere updated this revision to Diff 84581.
JDevlieghere marked 8 inline comments as done.
JDevlieghere added a comment.
- Added more tests
- Improved matchers
- Addressed code review comments from @malcolm.parsons, @Prazek and
@aaron.ballman
Repository:
rL LLVM
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:60
+ auto Diag =
+ diag(Loc, "use braced initializer list for constructing return types");
+
This diagnostic doesn't really tell the user what's wrong with
Prazek added a comment.
Thanks for the check. Have you run it on llvm?
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:27-32
+ auto soughtConstructExpr =
+ cxxConstructExpr(unless(isListInitialization())).bind("ctor");
+
+ auto hasConstructExpr =
malcolm.parsons added a comment.
`{}` doesn't allow narrowing; can you check for that?
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:50
+ SourceLocation Loc = MatchedConstructExpr->getExprLoc();
+ if (Loc.isMacroID())
+return;
Test?
JDevlieghere added a comment.
In https://reviews.llvm.org/D28768#647206, @malcolm.parsons wrote:
> In https://reviews.llvm.org/D28768#647204, @JDevlieghere wrote:
>
> > I wanted to do that but it seems that the test script hard codes it to
> > C++11, and the `auto` return type is a C++14
malcolm.parsons added a comment.
In https://reviews.llvm.org/D28768#647204, @JDevlieghere wrote:
> I wanted to do that but it seems that the test script hard codes it to C++11,
> and the `auto` return type is a C++14 feature. Maybe I'm mistaken though, but
> I didn't manage to get it working,
JDevlieghere marked 2 inline comments as done.
JDevlieghere added a comment.
In https://reviews.llvm.org/D28768#647203, @malcolm.parsons wrote:
> In https://reviews.llvm.org/D28768#647198, @JDevlieghere wrote:
>
> > In https://reviews.llvm.org/D28768#647177, @malcolm.parsons wrote:
> >
> > >
malcolm.parsons added a comment.
In https://reviews.llvm.org/D28768#647198, @JDevlieghere wrote:
> In https://reviews.llvm.org/D28768#647177, @malcolm.parsons wrote:
>
> > What happens if the function has `auto` as the return type?
>
>
> Nothing, the constructor is left untouched.
Please add a
JDevlieghere updated this revision to Diff 84543.
JDevlieghere added a comment.
- Fixed comments from @malcolm.parsons
In https://reviews.llvm.org/D28768#647177, @malcolm.parsons wrote:
> What happens if the function has `auto` as the return type?
Nothing, the constructor is left untouched.
malcolm.parsons added a comment.
What happens if the function has `auto` as the return type?
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:23
+void ReturnBracedInitListCheck::registerMatchers(MatchFinder *Finder) {
+ // Only register the matchers for C++.
+
JDevlieghere created this revision.
JDevlieghere added reviewers: hokein, Prazek, aaron.ballman, alexfh.
JDevlieghere added a subscriber: cfe-commits.
JDevlieghere set the repository for this revision to rL LLVM.
JDevlieghere added a project: clang-tools-extra.
Herald added a subscriber: mgorny.
33 matches
Mail list logo
