djasper added inline comments.
================
Comment at: include-fixer/IncludeFixer.cpp:234
@@ +233,3 @@
+ std::string MinimizedFilePath = minimizeInclude(
+ ((FilePath[0] == '"' || FilePath[0] == '<') ? FilePath
+ : "\"" + FilePath +
"\""),
----------------
How is this change related?
================
Comment at: include-fixer/IncludeFixer.cpp:238
@@ +237,3 @@
+ SymbolCandidates.emplace_back(Symbol.getName(), Symbol.getSymbolKind(),
+ MinimizedFilePath, Symbol.getLineNumber(),
+ Symbol.getContexts(),
----------------
Indentation is off.
================
Comment at: include-fixer/tool/ClangIncludeFixer.cpp:88
@@ -86,1 +87,3 @@
+cl::opt<bool> FixNamespace("fix-namespace",
+ cl::desc("Add missing namespace prefix to the "
----------------
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, ...
================
Comment at: unittests/include-fixer/IncludeFixerTest.cpp:235
@@ -223,1 +234,3 @@
+TEST(IncludeFixer, FixNamespace) {
+ EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
----------------
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)?
================
Comment at: unittests/include-fixer/IncludeFixerTest.cpp:253
@@ +252,3 @@
+
+ // FIXME: Fix-namespace should not add the missing namespace prefix to the
+ // unidentified symbol which is already in that namespace.
----------------
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.
http://reviews.llvm.org/D21603
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
