This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Refactoring of FixHintUtils
ClosedPublic

Authored by etienneb on May 2 2016, 8:17 AM.

Diff Detail

Event Timeline

etienneb updated this revision to Diff 55822.May 2 2016, 8:17 AM
etienneb retitled this revision from to [draft] Refactoring of FixHintUtils.
etienneb updated this object.
etienneb added a reviewer: alexfh.
alexfh added inline comments.
clang-tidy/misc/SwappedArgumentsCheck.cpp
51–52

Stray backslash at EOL.

clang-tidy/utils/FixItHintUtils.cpp
41

Please update the comment.

clang-tidy/utils/FixItHintUtils.h
21

These functions are not clang-tidy specific. They might be convenient in clang/include/clang/Tooling. Manuel, WDYT?

38

nit: functionsShouldStartWithALowerCase in LLVM.

38

Please document the functions. Specifically, we need to note that they can return a FixItHint that isNull in case the range is not a simple file range.

59–60

Please update the comment.

I'm mostly interested to validate the concept before pushing further the refactoring.

The same API is marked "deprecated" internally.
I just want to confirm I'm not missing an elephant in the room.

alexfh seems willing to push this as-is (modulo cleanup, packaging and tests)...

clang-tidy/misc/SwappedArgumentsCheck.cpp
51–52

yeah, a few nits.
I didn't format too. :)

clang-tidy/utils/FixItHintUtils.h
21

I didn't think about that yet?
Should we do this in two steps!?
First, lift them in clang-tidy to see the use cases.
Then, generalized them if possible and if it worth it?

I do not know enough on other tooling, yet!

59–60

I'm preparing a separate CL to rename these namespace too.
Forget about them in this patch.

etienneb updated this revision to Diff 55892.May 2 2016, 1:50 PM
etienneb marked 2 inline comments as done.

step

etienneb updated this revision to Diff 55893.May 2 2016, 1:52 PM

reformat

klimek added inline comments.May 3 2016, 1:07 AM
clang-tidy/utils/FixItHintUtils.h
21

If I'm not mistaken, this is only for template-cases to work, which are used in this file. I don't think this function carries its weight for a more general solution. I expect everybody else to just write SourceRange(Loc) - I don't think it's any less obvious than getSourceRange(Loc).

alexfh added inline comments.May 3 2016, 1:59 AM
clang-tidy/utils/FixItHintUtils.h
21

Yes, getSourceRange is needed for the the template getText function below. On its own it doesn't seem to be particularly useful and should probably be moved to the internal namespace.

The useful API here that makes sense to be moved to clang/Tooling (and that I was talking about) is the getText function and maybe also createRemoval and createReplacement.

Do you think they fit well to clang/Tooling? If so, what specific file would be the best place for them?

test/clang-tidy/misc-swapped-arguments.cpp
26

Please leave the CHECK-FIXES line to actually verify that this line is not changed.

klimek added inline comments.May 3 2016, 2:28 AM
clang-tidy/utils/FixItHintUtils.h
21

Ok, after short in-person chat I now understand what this is about:
we don't have good abstractions for ast-node based source abstractions (Replacements has this to some degree, but intentionally distances itself from the AST to be able to live in a small 'core' library).
Those might be a good idea to have, and should probably live in Tooling/AST.h or Tooling/ASTNodes.h or somesuch. Generally, Alex has convinced me he has a good design idea there, so I'll let him take it from here :)

etienneb added inline comments.May 4 2016, 11:04 AM
clang-tidy/utils/FixItHintUtils.h
38

This is really inconsistent accross the codebase! :(

return FixItHint::CreateRemoval(getSourceRange(Node));
                  ^^^^^^^^^^^^^^

But, I'm not against to follow any rules, in this case, the one on the website.

59–60

separate CL on-going.

etienneb marked an inline comment as done.May 4 2016, 11:36 AM
etienneb added inline comments.
test/clang-tidy/misc-swapped-arguments.cpp
26

This line was correctly rewrtited.
Turn out there is more logic under the GetText.
I need to write more use cases to validate if it's always doing the right thing.

etienneb updated this revision to Diff 56213.May 4 2016, 3:43 PM
etienneb marked an inline comment as done.

split the work

etienneb updated this revision to Diff 56909.May 11 2016, 7:18 AM

add unittests

etienneb retitled this revision from [draft] Refactoring of FixHintUtils to [clang-tidy] Refactoring of FixHintUtils.May 11 2016, 7:20 AM
etienneb updated this object.
alexfh accepted this revision.May 11 2016, 9:10 AM
alexfh edited edge metadata.

LG. Thank you for cleaning this up!

This revision is now accepted and ready to land.May 11 2016, 9:10 AM
alexfh edited edge metadata.May 11 2016, 9:11 AM
alexfh added a subscriber: cfe-commits.
etienneb closed this revision.May 11 2016, 10:44 AM