Page MenuHomePhabricator

[clang-tidy] refactor ExprSequence out of misc-use-after-move check
ClosedPublic

Authored by mnbvmar on Dec 12 2016, 11:20 PM.

Details

Summary

This diff moves ExprSequence helper structure out of misc/UseAfterMove.cpp source file so that it becomes available to all the checks. Now, new checks will profit from being able to find the potential sequences of expressions.

Notably, some day we might want to refactor the similar functionality from Sema in clang. It might require much more work, though.

Diff Detail

Event Timeline

mnbvmar updated this revision to Diff 81186.Dec 12 2016, 11:20 PM
mnbvmar updated this revision to Diff 81188.
mnbvmar retitled this revision from to [clang-tidy] refactor ExprSequence out of misc-use-after-move check.
mnbvmar updated this object.
mnbvmar changed the visibility from "No One" to "mnbvmar (Marek Sokołowski)".
mnbvmar updated this revision to Diff 81189.Dec 12 2016, 11:22 PM
mnbvmar changed the visibility from "mnbvmar (Marek Sokołowski)" to "Public (No Login Required)".

Fix "no-newline-at-the-end-of-file" message.

mnbvmar changed the edit policy from "No One" to "All Users".
mnbvmar edited subscribers, added: sbarzowski, Prazek, staronj, mnbvmar; removed: mgorny, JDevlieghere.
mnbvmar added a subscriber: cfe-commits.
Prazek added inline comments.Dec 13 2016, 3:04 AM
clang-tidy/misc/UseAfterMoveCheck.cpp
18

I don't think it is required

clang-tidy/utils/ExprSequence.cpp
181–183

same here

clang-tidy/utils/ExprSequence.h
122–124

Run clang-tidy llvm checks on this patch. These braces requires comments like

// namespace clang
Prazek added inline comments.Dec 13 2016, 3:07 AM
clang-tidy/misc/UseAfterMoveCheck.cpp
18

ok I guess I am wrong

alexfh added inline comments.Dec 13 2016, 6:02 AM
clang-tidy/utils/ExprSequence.cpp
155

nit: No else after return, please.

Prazek added inline comments.Dec 13 2016, 6:49 AM
clang-tidy/utils/ExprSequence.cpp
155

Not sure if he should change it in this patch - it is just move of this class to different file, so I am not sure if it is good do introduce small changes to it now.
I guess pushin NFC patch with this after would be good solution

alexfh added inline comments.Dec 13 2016, 7:27 AM
clang-tidy/utils/ExprSequence.cpp
155

For me it's usually easier to fix than to postpone to a different patch. Feel free to do either.

staronj added inline comments.Dec 13 2016, 7:34 AM
clang-tidy/utils/ExprSequence.cpp
53

Shouldn't isDescendantOrEqual be static or in inline namespace?

Prazek added inline comments.Dec 13 2016, 8:10 AM
clang-tidy/utils/ExprSequence.cpp
53

Goot catch. I guess putting it with getParentStmts into anonymous namespace is the best solution.
btw inline namespace is not the same as anonymous namespace :)

mboehme accepted this revision.Dec 14 2016, 8:41 AM
mboehme edited edge metadata.

LG apart from minor comments by others and me

clang-tidy/misc/UseAfterMoveCheck.cpp
18

I would suggest instead adding an explicit "utils::" qualifier -- it's only needed in two places anyway. I don't feel strongly about this though.

clang-tidy/utils/ExprSequence.cpp
53

Thanks for the catch -- this was already a bug in the original code...

180

Double newline

clang-tidy/utils/ExprSequence.h
121

Double newline

This revision is now accepted and ready to land.Dec 14 2016, 8:41 AM
mnbvmar updated this revision to Diff 81732.Dec 16 2016, 3:59 AM
mnbvmar edited edge metadata.
mnbvmar marked 2 inline comments as done.

Minor changes, according to the request.

mnbvmar marked 9 inline comments as done.Dec 16 2016, 4:00 AM
mnbvmar added inline comments.
clang-tidy/misc/UseAfterMoveCheck.cpp
18

It's actually needed in more places (I think at least four). I feel like leaving it here.

This revision was automatically updated to reflect the committed changes.
alexfh edited edge metadata.Jan 11 2017, 7:17 AM

One late comment.

clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp
154–155 ↗(On Diff #82440)

Use .find and store the iterator to avoid duplicate lookups.