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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
