Handle locations of nested macros better

We've seen that IWYU completely misses the mark when finding the use location
for htons from inet.h.

This patch should handle nested macros better. Test case based on htons added to
macro_location.cc

Fix #334 and #358.

4 files changed, 58 insertions, 37 deletions

diff --git a/iwyu.cc b/iwyu.cc
index 5c829e8..1c4297a 100644
--- a/iwyu.cc
+++ b/iwyu.cc
@@ -1375,62 +1375,61 @@ class IwyuBaseAstVisitor : public BaseAstVisitor<Derived> {
   SourceLocation GetCanonicalUseLocation(SourceLocation use_loc,
                                          const NamedDecl* decl) {
     // If we're not in a macro, just echo the use location.
-    if (!IsInMacro(use_loc))
+    if (!use_loc.isMacroID())
       return use_loc;
 
-    // If the macro definition file contains a forward-declaration for decl,
-    // we treat that as a use-attribution hint. See below for details.
-    const FileEntry* macro_def_file = GetFileEntry(GetSpellingLoc(use_loc));
+    VERRS(5) << "Trying to determine use location for '"
+             << PrintableDecl(decl) << "'\n";
+
+    clang::SourceManager* sm = GlobalSourceManager();
+    SourceLocation spelling_loc = sm->getSpellingLoc(use_loc);
+    SourceLocation expansion_loc = sm->getExpansionLoc(use_loc);
+
+    // If the file defining the macro contains a forward decl, keep it around
+    // and treat it as a hint that the expansion loc is responsible for the
+    // symbol.
+    const FileEntry* macro_def_file = GetLocFileEntry(spelling_loc);
+    VERRS(5) << "Macro is defined in file '" << GetFilePath(macro_def_file)
+             << "'. Looking for fwd-decl hint...\n";
+
     const NamedDecl* fwd_decl = nullptr;
     for (const NamedDecl* redecl : GetClassRedecls(decl)) {
-      if (GetFileEntry(redecl) == macro_def_file &&
-          IsForwardDecl(redecl)) {
+      if (GetFileEntry(redecl) == macro_def_file && IsForwardDecl(redecl)) {
         fwd_decl = redecl;
+
+        // Make sure we keep that forward-declaration, even if it's probably
+        // unused in this file.
+        IwyuFileInfo* file_info =
+            preprocessor_info().FileInfoFor(macro_def_file);
+        file_info->ReportForwardDeclareUse(
+            spelling_loc, fwd_decl,
+            IsNodeInsideCXXMethodBody(current_ast_node()), nullptr);
         break;
       }
     }
 
-    if (fwd_decl) {
-      // Make sure we keep that forward-declaration, even if it's probably
-      // unused in this file.
-      preprocessor_info().FileInfoFor(macro_def_file)->ReportForwardDeclareUse(
-          use_loc, decl, IsNodeInsideCXXMethodBody(current_ast_node()),
-          nullptr);
-    }
-
     // Resolve the best use location based on our current knowledge.
     //
-    // 1) If the macro definition file forward-declares the used decl, that's a
-    //    hint that it wants the expansion location to take responsibility.
-    // 2) If the symbol being used is passed as an argument to the macro, we
-    //    want the expansion location to take responsibility (that thing they
-    //    passed in is something they know about, and the macro doesn't.)
-    // 3) If the use_loc is in <scratch space>, we assume it's formed by macro
+    // 1) If the use_loc is in <scratch space>, we assume it's formed by macro
     //    argument concatenation, and attribute responsibility to the expansion
     //    location.
+    // 2) If the macro definition file forward-declares the used decl, that's a
+    //    hint that it wants the expansion location to take responsibility.
     //
-    // Otherwise, the macro file is responsible for including the symbol.
-    SourceLocation expansion_loc = GetInstantiationLoc(use_loc);
-    const char* responsible = "expansion";
-
-    if (fwd_decl != nullptr) {
-      VERRS(5) << "Macro file has a forward-decl attribution hint.\n";
-      use_loc = expansion_loc;
-    } else if (IsInScratchSpace(use_loc)) {
-      VERRS(5) << "Decl was used in <scratch space>, presumably as a result of "
-                  "macro arg concatenation.\n";
+    // Otherwise, the spelling loc is responsible.
+    if (IsInScratchSpace(spelling_loc)) {
+      VERRS(5) << "Spelling location is in <scratch space>, presumably as a "
+                  "result of macro arg concatenation.\n";
       use_loc = expansion_loc;
-    } else if (GlobalSourceManager()->isMacroArgExpansion(use_loc)) {
-      VERRS(5) << "Use location is a macro arg expansion.\n";
+    } else if (fwd_decl != nullptr) {
+      VERRS(5) << "Found a forward-decl in macro definition file.\n";
       use_loc = expansion_loc;
     } else {
-      use_loc = GetSpellingLoc(use_loc);
-      responsible = "definition";
+      use_loc = spelling_loc;
     }
 
-    VERRS(5) << "Attributing " << decl->getNameAsString() << " use to macro "
-             << responsible << " location at " << PrintableLoc(use_loc)
-             << ".\n";
+    VERRS(4) << "Attributing use of '" << PrintableDecl(decl)
+             << "' to location at " << PrintableLoc(use_loc) << ".\n";
 
     return use_loc;
   }
diff --git a/tests/cxx/macro_location-byteswap.h b/tests/cxx/macro_location-byteswap.h
new file mode 100644
index 0000000..921cf28
--- /dev/null
+++ b/tests/cxx/macro_location-byteswap.h
@@ -0,0 +1,2 @@
+#define bswap(x) ({ int __x = (x); bswap2(__x); })
+#define bswap2(x) (x)
diff --git a/tests/cxx/macro_location-inet.h b/tests/cxx/macro_location-inet.h
new file mode 100644
index 0000000..f19e3fb
--- /dev/null
+++ b/tests/cxx/macro_location-inet.h
@@ -0,0 +1,12 @@
+//===--- macro_location-inet.h - test input file for iwyu -----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "tests/cxx/macro_location-byteswap.h"
+
+#define htons(x) bswap(x)
diff --git a/tests/cxx/macro_location.cc b/tests/cxx/macro_location.cc
index 27f7a92..032ca06 100644
--- a/tests/cxx/macro_location.cc
+++ b/tests/cxx/macro_location.cc
@@ -13,6 +13,7 @@
 // expansions when the macro is written in a to-ignore file.
 
 #include "tests/cxx/macro_location.h"
+#include "tests/cxx/macro_location-inet.h"
 
 struct A {
   // This doesn't require us to forward-declare ToBeDeclared because it's
@@ -27,6 +28,13 @@ struct A {
 class ToBeDeclaredLater1 { };
 class ToBeDeclaredLater2 { };
 
+// This is lifted from a reduced repro case for htons of inet.h.
+// Its nesting provokes something that isn't covered by the rest of the
+// macro_location suite.
+int my_htons(int x) {
+  return htons(x);
+}
+
 /**** IWYU_SUMMARY
 
 (tests/cxx/macro_location.cc has correct #includes/fwd-decls)