[PATCH] D21603: [include-fixer] Add missing namespace qualifiers after inserting a missing header.

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: include-fixer/IncludeFixer.cpp:224
@@ +223,3 @@
+      std::string MinimizedFilePath = minimizeInclude(
+          ((FilePath[0] == '"' || FilePath[0] == '<') ? FilePath
+                                                      : "\"" + FilePath + "\""),
----------------
Ah, I see. This was just moved. Sorry, missed that.

================
Comment at: include-fixer/IncludeFixer.cpp:258
@@ +257,3 @@
+
+    // Query the symbol based on C++ name Lookup rules.
+    // Firstly, lookup the identifier with scoped namespace contexts; If fails,
----------------
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.

================
Comment at: include-fixer/IncludeFixerContext.h:31
@@ +30,3 @@
+        MatchedSymbols(Symbols), SymbolRange(Range) {
+    // Deduplicate headers, so that we don't want to suggest the same header
+    // twice.
----------------
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?

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.cpp:93
@@ -92,1 +92,3 @@
 
+std::string SymbolInfo::getFullyQualifiedName() const {
+  std::string FullyQuanlifiedName = Name;
----------------
I think "getQualifiedName" is sufficient and then matches what's in NamedDecl.

================
Comment at: include-fixer/find-all-symbols/SymbolInfo.cpp:94
@@ +93,3 @@
+std::string SymbolInfo::getFullyQualifiedName() const {
+  std::string FullyQuanlifiedName = Name;
+  for (const auto &Context : Contexts) {
----------------
There is an extra "n" in "FullyQuanlifiedName".

================
Comment at: include-fixer/tool/ClangIncludeFixer.cpp:88
@@ -86,1 +87,3 @@
 
+cl::opt<bool> FixNamespaceQualifiers("fix-namespace-qualifiers",
+                           cl::desc("Add missing namespace qualifiers to the "
----------------
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.

================
Comment at: unittests/include-fixer/IncludeFixerTest.cpp:144
@@ -141,1 +143,3 @@
+            runIncludeFixer("a::b::foo bar;\n",
+                            /*FixNamespaceQualifiers=*/false, IncludePath));
 
----------------
Do you need to set FixNamespaceQualifiers to false here?


http://reviews.llvm.org/D21603