[PATCH] D115183: [clang][HeaderSearch] Support framework includes in suggestPath...

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty nice to me, but I'm not convinced it's a good idea to include the umbrella header heuristic.



================
Comment at: clang/lib/Lex/HeaderSearch.cpp:782
     if (IsIncludeeInFramework) {
-      NewInclude += ToFramework.str().drop_back(10); // drop .framework
-      NewInclude += "/";
+      NewInclude += ToIncludeSpelling;
+      NewInclude += ">";
----------------
Nice!

No good deed goes un-punished: it should be possible to add a diagnostic test?
There are some under clang/test/Preprocessor/*framework*


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1862
+  // `BestPrefixLength` accordingly.
+  auto CheckDir = [&](llvm::StringRef Dir, bool IsFramework) -> bool {
     llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
----------------
Seems like it'd be more consistent with the existing IsSystem to expand the callsites as

```
if (... && CheckDir(...)) {
  if (IsSystem)
    *IsSystem = whatever;
  IsFramework = whatever;
}
```

instead of taking a param here. (And honestly the CheckDir() && IsSystem thing is confusing, so it'd help)


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1899
+      // symlink like `iPhoneSimulator14.5.sdk` while the file is instead
+      // located in `iPhoneSimulator.sdk` (the real folder).
+      if (NI->endswith(".sdk") && DI->endswith(".sdk")) {
----------------
Nice comment, very helpful!

Just wanted to check you're sure the versioned one is the symlink and the unversioned one is the real folder, rather than the other way. (Plausible, just surprising to me)


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1916
+      StringRef Dir = SearchDirs[I].getDir()->getName();
+      if (CheckDir(Dir, /** IsFramework= */ false) && IsSystem)
+        *IsSystem = BestPrefixLength ? I >= SystemDirIdx : false;
----------------
nit: `/*IsFramework=*/`, one star no spaces


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1962
+    } else {
+      // Prefer using `<UIKit/UIKit.h>` for non-`UIKit` main files.
+      //
----------------
This heuristic looks iffy to me, it's not just trying to help the caller insert the right header, but tell them they want a different one for style reasons.

The first example framework in https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html doesn't include an umbrella header of this form.

Moreover, non-framework libraries have umbrella headers too, this seems like an orthogonal concern best caught in some other way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115183