[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2023-01-20 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added a comment.

In D119708#4062632 , @jansvoboda11 
wrote:

> If the plan is to eventually upstream that part of Cling, I'm fine with 
> re-adding a safe version of this API.

I'll be honest here and say that I personally find it unlikely that this part 
of ROOT will be upstreamed, it is very specific and also there for 
backwards-compatibility in a very specific case. Nevertheless, I understand 
that Clang should offer its clients a rich API to get all information they 
need, so I put together https://reviews.llvm.org/D142196 with a hopefully 
"safe" version of a `FileNotFound` callback, that optionally even supports 
silently skipping this file. This would provide exactly what we need.

Another possibility that I considered is extending `InclusionDirective` and 
always call it, also for missing files. However, I find this much less clear 
and we already have `FileSkipped`...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2023-01-18 Thread Jan Svoboda via Phabricator via cfe-commits 
jansvoboda11 added a comment.

If the plan is to eventually upstream that part of Cling, I'm fine with 
re-adding a safe version of this API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2023-01-18 Thread Vassil Vassilev via Phabricator via cfe-commits 
v.g.vassilev added a comment.

In D119708#4060336 , @jansvoboda11 
wrote:

> In D119708#4059254 , @Hahnfeld 
> wrote:
>
>> Hello, sorry for the late heads-up, but this functionality is used by ROOT: 
>> https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282
>>  Any chance of bringing this back?
>
> Hi. Your downstream callback never fills in `RecoveryPath` and always returns 
> `false`, meaning it will never reach the unsafe block of code I had issue 
> with: `HeaderInfo.AddSearchPath(DL, isAngled)`.
> Technically, we could bring back a safe/trimmed-down version of this callback 
> that doesn't take the out-parameter and returns `void`. But I don't think 
> it's the right call, since that would essentially re-introduce dead code to 
> upstream.
> This is probably best kept downstream, unless there are other downstream 
> users.

+1 for bringing back that piece of code because it seems an interesting event 
to listen to anyways. Would you be more comfortable if we provide also a test 
case and relevant comments explaining the use-case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2023-01-17 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added a comment.

In D119708#4060336 , @jansvoboda11 
wrote:

> In D119708#4059254 , @Hahnfeld 
> wrote:
>
>> Hello, sorry for the late heads-up, but this functionality is used by ROOT: 
>> https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282
>>  Any chance of bringing this back?
>
> Hi. Your downstream callback never fills in `RecoveryPath` and always returns 
> `false`, meaning it will never reach the unsafe block of code I had issue 
> with: `HeaderInfo.AddSearchPath(DL, isAngled)`.

This is true, but it provides us a hook to do something in case a file is not 
found. For ROOT, we "extend" the `#include` syntax and allow "modifiers" at the 
end. Then we do something special (completely on our own) for things like 
`#include "myfile.C+"` (note the plus).

> Technically, we could bring back a safe/trimmed-down version of this callback 
> that doesn't take the out-parameter and returns `void`. But I don't think 
> it's the right call, since that would essentially re-introduce dead code to 
> upstream.
> This is probably best kept downstream, unless there are other downstream 
> users.

Sure, but how would we do that without a hook? At the moment, we try very hard 
to reduce the number of patches that we carry for Clang (which is also why 
parts of Cling are upstreamed into `clang-repl`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2023-01-17 Thread Jan Svoboda via Phabricator via cfe-commits 
jansvoboda11 added a comment.

In D119708#4059254 , @Hahnfeld wrote:

> Hello, sorry for the late heads-up, but this functionality is used by ROOT: 
> https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282
>  Any chance of bringing this back?

Hi. Your downstream callback never fills in `RecoveryPath` and always returns 
`false`, meaning it will never reach the unsafe block of code I had issue with: 
`HeaderInfo.AddSearchPath(DL, isAngled)`.
Technically, we could bring back a safe/trimmed-down version of this callback 
that doesn't take the out-parameter and returns `void`. But I don't think it's 
the right call, since that would essentially re-introduce dead code to upstream.
This is probably best kept downstream, unless there are other downstream users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2023-01-17 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added subscribers: v.g.vassilev, Hahnfeld.
Hahnfeld added a comment.
Herald added a subscriber: ributzka.
Herald added a project: All.

Hello, sorry for the late heads-up, but this functionality is used by ROOT: 
https://github.com/root-project/root/blob/f58cccf5ce7fd67894c7fd9e9e74d3f37bc1acba/core/metacling/src/TClingCallbacks.cxx#L282
 Any chance of bringing this back?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2022-02-15 Thread Jan Svoboda via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7a124f4859d5: [clang][lex] Remove 
`PPCallbacks::FileNotFound()` (authored by jansvoboda11).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

Files:
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Lex/PPDirectives.cpp

Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1845,28 +1845,6 @@
   if (File)
 return File;
 
-  if (Callbacks) {
-// Give the clients a chance to recover.
-SmallString<128> RecoveryPath;
-if (Callbacks->FileNotFound(Filename, RecoveryPath)) {
-  if (auto DE = FileMgr.getOptionalDirectoryRef(RecoveryPath)) {
-// Add the recovery path to the list of search paths.
-DirectoryLookup DL(*DE, SrcMgr::C_User, false);
-HeaderInfo.AddSearchPath(DL, isAngled);
-
-// Try the lookup again, skipping the cache.
-Optional File = LookupFile(
-FilenameLoc,
-LookupFilename, isAngled,
-LookupFrom, LookupFromFile, CurDir, nullptr, nullptr,
-, , /*IsFrameworkFound=*/nullptr,
-/*SkipCache*/ true);
-if (File)
-  return File;
-  }
-}
-  }
-
   if (SuppressIncludeNotFoundError)
 return None;
 
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -61,23 +61,6 @@
const Token ,
SrcMgr::CharacteristicKind FileType) {}
 
-  /// Callback invoked whenever an inclusion directive results in a
-  /// file-not-found error.
-  ///
-  /// \param FileName The name of the file being included, as written in the
-  /// source code.
-  ///
-  /// \param RecoveryPath If this client indicates that it can recover from
-  /// this missing file, the client should set this as an additional header
-  /// search patch.
-  ///
-  /// \returns true to indicate that the preprocessor should attempt to recover
-  /// by adding \p RecoveryPath as a header search path.
-  virtual bool FileNotFound(StringRef FileName,
-SmallVectorImpl ) {
-return false;
-  }
-
   /// Callback invoked whenever an inclusion directive of
   /// any kind (\c \#include, \c \#import, etc.) has been processed, regardless
   /// of whether the inclusion will actually result in an inclusion.
@@ -443,12 +426,6 @@
 Second->FileSkipped(SkippedFile, FilenameTok, FileType);
   }
 
-  bool FileNotFound(StringRef FileName,
-SmallVectorImpl ) override {
-return First->FileNotFound(FileName, RecoveryPath) ||
-   Second->FileNotFound(FileName, RecoveryPath);
-  }
-
   void InclusionDirective(SourceLocation HashLoc, const Token ,
   StringRef FileName, bool IsAngled,
   CharSourceRange FilenameRange, const FileEntry *File,
Index: clang-tools-extra/pp-trace/PPCallbacksTracker.h
===
--- clang-tools-extra/pp-trace/PPCallbacksTracker.h
+++ clang-tools-extra/pp-trace/PPCallbacksTracker.h
@@ -91,8 +91,6 @@
FileID PrevFID = FileID()) override;
   void FileSkipped(const FileEntryRef , const Token ,
SrcMgr::CharacteristicKind FileType) override;
-  bool FileNotFound(llvm::StringRef FileName,
-llvm::SmallVectorImpl ) override;
   void InclusionDirective(SourceLocation HashLoc, const Token ,
   llvm::StringRef FileName, bool IsAngled,
   CharSourceRange FilenameRange, const FileEntry *File,
Index: clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
===
--- clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
+++ clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
@@ -128,16 +128,6 @@
   appendArgument("FileType", FileType, CharacteristicKindStrings);
 }
 
-// Callback invoked whenever an inclusion directive results in a
-// file-not-found error.
-bool
-PPCallbacksTracker::FileNotFound(llvm::StringRef FileName,
- llvm::SmallVectorImpl ) {
-  beginCallback("FileNotFound");
-  appendFilePathArgument("FileName", FileName);
-  return false;
-}
-
 // Callback invoked whenever an inclusion directive of
 // any kind (#include, #import, etc.) has been processed, regardless
 // of whether the inclusion will actually result in an inclusion.
Index: clang-tools-extra/clangd/ParsedAST.cpp

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2022-02-14 Thread Alex Hoppen via Phabricator via cfe-commits 
ahoppen accepted this revision.
ahoppen added a comment.
This revision is now accepted and ready to land.

Assuming that this is indeed never used, removing dead code always sounds good 
to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

2022-02-14 Thread Jan Svoboda via Phabricator via cfe-commits 
jansvoboda11 created this revision.
jansvoboda11 added reviewers: ahoppen, Bigcheese, dexonsmith.
Herald added subscribers: usaxena95, shchenz, kadircet, arphaman, kbarton, 
nemanjai.
jansvoboda11 requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

The purpose of the `FileNotFound` preprocessor callback was to add the ability 
to recover from failed header lookups. This was to support downstream project.

However, injecting additional search path while performing header search can 
invalidate currently used iterators/references to `DirectoryLookup` in 
`Preprocessor` and `HeaderSearch`.

The downstream project ended up maintaining a separate patch to further tweak 
the functionality. Since we don't have any upstream users nor open source 
downstream users, I'd like to remove this callback for good to prevent future 
misuse. I doubt there are any actual downstream users, since the functionality 
is definitely broken at the moment.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119708

Files:
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
  clang-tools-extra/pp-trace/PPCallbacksTracker.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Lex/PPDirectives.cpp

Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1845,28 +1845,6 @@
   if (File)
 return File;
 
-  if (Callbacks) {
-// Give the clients a chance to recover.
-SmallString<128> RecoveryPath;
-if (Callbacks->FileNotFound(Filename, RecoveryPath)) {
-  if (auto DE = FileMgr.getOptionalDirectoryRef(RecoveryPath)) {
-// Add the recovery path to the list of search paths.
-DirectoryLookup DL(*DE, SrcMgr::C_User, false);
-HeaderInfo.AddSearchPath(DL, isAngled);
-
-// Try the lookup again, skipping the cache.
-Optional File = LookupFile(
-FilenameLoc,
-LookupFilename, isAngled,
-LookupFrom, LookupFromFile, CurDir, nullptr, nullptr,
-, , /*IsFrameworkFound=*/nullptr,
-/*SkipCache*/ true);
-if (File)
-  return File;
-  }
-}
-  }
-
   if (SuppressIncludeNotFoundError)
 return None;
 
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -61,23 +61,6 @@
const Token ,
SrcMgr::CharacteristicKind FileType) {}
 
-  /// Callback invoked whenever an inclusion directive results in a
-  /// file-not-found error.
-  ///
-  /// \param FileName The name of the file being included, as written in the
-  /// source code.
-  ///
-  /// \param RecoveryPath If this client indicates that it can recover from
-  /// this missing file, the client should set this as an additional header
-  /// search patch.
-  ///
-  /// \returns true to indicate that the preprocessor should attempt to recover
-  /// by adding \p RecoveryPath as a header search path.
-  virtual bool FileNotFound(StringRef FileName,
-SmallVectorImpl ) {
-return false;
-  }
-
   /// Callback invoked whenever an inclusion directive of
   /// any kind (\c \#include, \c \#import, etc.) has been processed, regardless
   /// of whether the inclusion will actually result in an inclusion.
@@ -443,12 +426,6 @@
 Second->FileSkipped(SkippedFile, FilenameTok, FileType);
   }
 
-  bool FileNotFound(StringRef FileName,
-SmallVectorImpl ) override {
-return First->FileNotFound(FileName, RecoveryPath) ||
-   Second->FileNotFound(FileName, RecoveryPath);
-  }
-
   void InclusionDirective(SourceLocation HashLoc, const Token ,
   StringRef FileName, bool IsAngled,
   CharSourceRange FilenameRange, const FileEntry *File,
Index: clang-tools-extra/pp-trace/PPCallbacksTracker.h
===
--- clang-tools-extra/pp-trace/PPCallbacksTracker.h
+++ clang-tools-extra/pp-trace/PPCallbacksTracker.h
@@ -91,8 +91,6 @@
FileID PrevFID = FileID()) override;
   void FileSkipped(const FileEntryRef , const Token ,
SrcMgr::CharacteristicKind FileType) override;
-  bool FileNotFound(llvm::StringRef FileName,
-llvm::SmallVectorImpl ) override;
   void InclusionDirective(SourceLocation HashLoc, const Token ,
   llvm::StringRef FileName, bool IsAngled,
   CharSourceRange FilenameRange, const FileEntry *File,
Index: clang-tools-extra/pp-trace/PPCallbacksTracker.cpp
===
---