[PATCH] D123831: [clang][extract-api] Use relative includes

[Daniel Grumberg via Phabricator via cfe-commits]

dang added inline comments.


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:134
+        if (!SpelledFilename.empty())
+          return SpelledFilename.str();
+
----------------
zixuw wrote:
> zixuw wrote:
> > zixuw wrote:
> > > One problem I can see in this right now is that there might be multiple headermaps that together construct a chain of mapping, for example
> > > ```
> > > // A.hmap
> > > Header.h -> Product/Header.h
> > > 
> > > // B.hmap
> > > Product/Header.h -> /Absolute/path/to/Header.h
> > > ```
> > > Then this iteration would conclude on using `Product/Header.h` and missed the first mapping (if the command line goes `clang -extract-api -I A.hmap -I B.hmap ...`).
> > > 
> > > It's also possible that a headermap and a search path together resolve the header. For example:
> > > ```
> > > //A.hmap
> > > Header.h -> Product/Header.h
> > > 
> > > // Invocation:
> > > clang -extract-api /Some/Path/to/Product/Header.h -I A.hmap -I /Some/path/to/
> > > ```
> > > 
> > > I think we need to keep records and iterate backwards on the search paths again to get the full reverse lookup
> > Or, iterate once in reverse and find the last match? 🤔 
> Another thing just came to me: with multiple headermaps chaining remaps, which is the "correct" way to include a header?
> ```
> // A.hmap
> Header.h -> Product/Header.h
> 
> // B.hmap
> Product/Header.h -> /Absolute/path/to/Header.h
> ```
> In the context of Darwin frameworks, we'd use `<Framework/Header.h>` so we don't want to follow the chain all the way backwards.
> But without any context or build system rationales of why multiple headermaps with remap chains are generated in the first place, I'm feeling that we don't nearly have enough information for this if we're only talking about headermap as it is and refraining from adopting heuristics.
Two things:
- If we want an exhaustive search, then it would make sense to do what would actually happen in a compilation. Iterate forward and find all matches, then iterate forwards again with each of the matches from the previous step until we find all terminal matches and then heuristically pick the "best one" probably the shortest one.
- The "correct" way of including a header would certainly be `#include <Product/Header.h>` for a Darwin framework. Nonetheless if the search paths and headermaps setup means that `#include "Header.h"` or `#include <Header.h>` is a possible way of getting to the same file I see no harm in doing that. As long as shortening a given file results in deterministic results it should work fine. I think it would be a user error if shortening a file path to say `<Header.h>` meant that including it would lead to a completely different file (that is with different declarations/definitions).


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:908
+    if (auto RelativeName = getRelativeIncludeName(CI, FilePath)) {
+      HeaderContents += " <";
+      HeaderContents += *RelativeName;
----------------
I think `getRelativeIncludeName` is slightly wrong. It doesn't seem like it accounts for matches specifically made with `-iquote` arguments. My understanding is that we should record that information at that point and include the file using the appropriate form `#include "RelativeName"` or `#include <RelativeName>`. It would probably be best if `getRelativeIncludeName` included the quote type in the string directly.


================
Comment at: clang/test/ExtractAPI/known_files_only_hmap.c:1
+// XFAIL: *
 // RUN: rm -rf %t
----------------
I think you can safely delete this test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123831/new/

https://reviews.llvm.org/D123831