This is an archive of the discontinued LLVM Phabricator instance.

[include-fixer] Add missing namespace qualifiers after inserting a missing header.
ClosedPublic

Authored by hokein on Jun 22 2016, 3:10 AM.

Details

Summary

This is an initial version of fixing namespace issues by adding missing
namespace qualifiers to an unidentified symbol.

This version only fixes the first discovered unidentified symbol.
In the long run, include-fixer should fix all unidentified symbols
with a same name at one run.

Currently, it works on command-line tool. The vim integration is not
implemented yet.

Diff Detail

Event Timeline

hokein updated this revision to Diff 61523.Jun 22 2016, 3:10 AM
hokein retitled this revision from to [include-fixer] Fix namespace after inserting a missing header..
hokein updated this object.
hokein added a subscriber: cfe-commits.
hokein updated this revision to Diff 61524.Jun 22 2016, 3:13 AM

Fix a typo.

hokein updated this object.Jun 22 2016, 3:16 AM
hokein added reviewers: klimek, djasper.
hokein added subscribers: ioeric, bkramer.
djasper edited edge metadata.Jun 28 2016, 3:06 AM

Sorry, I completely forgot about this. Will try to review today. Is this part about the patch description accurate?

"This version only fixes the first discovered unidentified symbol.
In the long run, include-fixer should fix all unidentified symbols
with a same name at one run.

Currently, it works on command-line tool. The vim integration is not
implemented yet."

Specifically, what needs to be implemented in vim to make this work?

Sorry, I completely forgot about this. Will try to review today. Is this part about the patch description accurate?

Yes, the description is accurate.

Specifically, what needs to be implemented in vim to make this work?

We need a way to dump the mapping relationship between symbols and their headers, so that include-fixer can know which namespace prefix should be used when user selects a particular header in vim.

djasper added inline comments.Jun 30 2016, 4:35 AM
include-fixer/IncludeFixer.cpp
224

How is this change related?

228

Indentation is off.

include-fixer/tool/ClangIncludeFixer.cpp
88

Do we really want this? Should we just always do it? Who is going to use this flag?

If we want it, maybe call it "fix-namespace-qualifiers"? This renaming might also make sense of the patch itself, the test case, ...

unittests/include-fixer/IncludeFixerTest.cpp
231

All the tests in here test with an existing NestedNameSpecifier (i.e. b::bar). Is there a reason to not also have tests with a plain identifier (e.g. bar)?

249

I think we should address this part from the start. Otherwise, we are making the current behavior worse for a significant amount of cases. I suspect that frequently people use types from their own projects (and thus in the same namespace they are already in) and we don't want to add nested name specifiers for those.

hokein retitled this revision from [include-fixer] Fix namespace after inserting a missing header. to [include-fixer] Add missing namespace qualifiers after inserting a missing header..Jun 30 2016, 10:39 AM
hokein updated this object.
hokein edited edge metadata.
hokein updated this revision to Diff 62383.Jun 30 2016, 10:55 AM
hokein marked 4 inline comments as done.

Address review comments.

hokein added inline comments.Jun 30 2016, 10:58 AM
include-fixer/IncludeFixer.cpp
224

We need to check whether the FilePath contains " here because the FilePath in symbol indexer doesn't contains <, but for standard headers, it contains "<".

include-fixer/tool/ClangIncludeFixer.cpp
88

I don't have many ideas about this, but I'd like to add this option at the moment, so that user can enable/disable this feature freely. If we want, we could remove it in the future.

djasper added inline comments.Jul 1 2016, 12:36 AM
include-fixer/IncludeFixer.cpp
224

Ah, I see. This was just moved. Sorry, missed that.

258

Could you add something about this to the CL description? Or actually, I think this is very separate from fixing the namespace qualifiers. Can we move this part to a different patch?

Or am I missing something so that one cannot go without the other? Fundamentally, this seems to be doing two things:

  1. Insert namespace qualifiers if they are missing.
  2. Take existing namespace qualifiers to ensure we aren't looking up the wrong symbol.
include-fixer/IncludeFixerContext.h
31

This again seems like something that is unrelated to what the patch is actually meant to do. Can we split this into a separate patch?

include-fixer/find-all-symbols/SymbolInfo.cpp
93

I think "getQualifiedName" is sufficient and then matches what's in NamedDecl.

94

There is an extra "n" in "FullyQuanlifiedName".

include-fixer/tool/ClangIncludeFixer.cpp
88

I'd strongly recommend not doing that. Removing flags that people are using is hard (breaks them). Make this good enough so that it is always the right thing to do.

unittests/include-fixer/IncludeFixerTest.cpp
144

Do you need to set FixNamespaceQualifiers to false here?

hokein updated this revision to Diff 62473.Jul 1 2016, 1:34 AM
hokein marked 5 inline comments as done.

Fix review comments.

include-fixer/IncludeFixer.cpp
258

It's not introduced by this patch. Actually it moves from query function caller to the function body. Why we make this change? Because we need to save the scoped qualifiers of the symbol, so that include-fixer can know how to create a correct qualifiers for the symbol.

include-fixer/IncludeFixerContext.h
31

This is also a move from rankByPopularity in SymbolIndexManager.cpp.

unittests/include-fixer/IncludeFixerTest.cpp
144

No need to. Both are OK. Change to true here.

In the future I'd prefer to have patches like this split up in a part that refactors and a part that contains the actual change. Having that in one patch makes it really hard to review.

include-fixer/IncludeFixer.cpp
73

std::string, otherwise this will be a user after free.

include-fixer/IncludeFixerContext.h
76

Explain what a scoped qualifier is in this comment.

86

typo: breif

hokein updated this revision to Diff 62849.Jul 6 2016, 5:20 AM
hokein marked 2 inline comments as done.

Address Ben's comments.

hokein added a comment.Jul 6 2016, 5:21 AM

In the future I'd prefer to have patches like this split up in a part that refactors and a part that contains the actual change. Having that in one patch makes it really hard to review.

Acknowledged. I'm sorry for it. I didn't foresee that I would change so much code before I implementing this.

So let me summarize the changes of this patch:

  1. Instead of returning raw string headers, make SymbolIndexManager::search return Symbolnfos.
  2. Move rankByPopularity unique SymbolInfo code from SymbolIndexManager.cpp to IncludeFixerContext.cpp.
  3. Extend IncludeFixerContext to track more SymbolInfo information (e.g. SymbolRange, SymbolScopedQualifiers).
  4. Some move stuff in Action::query API.
  5. Create adding namespace qualifiers replacement for the unidentified symbol.
include-fixer/IncludeFixer.cpp
73

Good catch. Done.

bkramer accepted this revision.Jul 6 2016, 5:59 AM
bkramer added a reviewer: bkramer.

I think this can go in now.

This revision is now accepted and ready to land.Jul 6 2016, 5:59 AM
djasper added inline comments.Jul 6 2016, 6:27 AM
include-fixer/IncludeFixer.cpp
166

Lots of code duplication. Maybe pull out function/lambda?

259

"If that fails, .."

unittests/include-fixer/IncludeFixerTest.cpp
144

I think we should set this to true everywhere or more precisely completely remove this parameter. Now that the flag is gone, we are starting to test implementation details IMO.

hokein updated this revision to Diff 63043.Jul 7 2016, 2:16 AM
hokein marked 3 inline comments as done.
hokein edited edge metadata.
  • Address Daniel's comments.
  • Add tests for nested classes.
hokein added inline comments.Jul 7 2016, 2:17 AM
unittests/include-fixer/IncludeFixerTest.cpp
144

Yeah, this parameter is not needed. Removed it.

djasper accepted this revision.Jul 7 2016, 2:44 AM
djasper edited edge metadata.

Looks good.

This revision was automatically updated to reflect the committed changes.