[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Thanks for the reviews; pushed f4d02fbe418db55375b78b8f57e47126e4642fb6 
.




Comment at: clang/include/clang/Frontend/PrecompiledPreamble.h:108
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds 
Bounds,
-llvm::vfs::FileSystem *VFS) const;
+const llvm::MemoryBufferRef ,
+PreambleBounds Bounds, llvm::vfs::FileSystem ) const;

kadircet wrote:
> dexonsmith wrote:
> > jansvoboda11 wrote:
> > > dexonsmith wrote:
> > > > kadircet wrote:
> > > > > why not accept a value directly here? (i.e. drop const and ref)
> > > > Ah, yes, I've done this a few times, and it still seems not quite 
> > > > right. But the alternative also doesn't feel right when it's not 
> > > > necessary:
> > > > ```
> > > > #include "llvm/Basic/MemoryBufferRef.h"
> > > > ```
> > > > I'm happy either way since that file is fairly careful to avoid 
> > > > bloating includes.
> > > I agree this looks a bit odd, but avoiding an unnecessary include seems 
> > > like a good excuse.
> > @kadircet , WDYT?
> sorry i was on vacation and just got the chance to get back to this.
> 
> I don't feel so bad about the include, as the header itself is small-ish and 
> only includes StringRef.h, which is already included by this header. So I 
> would lean towards accepting this by value and keeping the API clean, rather 
> than trying to get away with a forward declaration.
> 
> but definitely up to you, i don't feel strongly about it in any case. (as you 
> can easily make the argument of header for MemoryBufferRef getting bloated 
> over time)
I decided to leave it as-is for now to match the other `MemoryBufferRef` APIs 
in the file... but I take your point that `MemoryBufferRef` is a cheap include 
and I expect it will stay cheap. I'll aim to circle back and update the whole 
file at some point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf4d02fbe418d: Frontend: Take VFS and MainFileBuffer by 
reference in PrecompiledPreamble… (authored by dexonsmith).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91297

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp

Index: clang/lib/Frontend/PrecompiledPreamble.cpp
===
--- clang/lib/Frontend/PrecompiledPreamble.cpp
+++ clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -501,12 +501,12 @@
 }
 
 bool PrecompiledPreamble::CanReuse(const CompilerInvocation ,
-   const llvm::MemoryBuffer *MainFileBuffer,
+   const llvm::MemoryBufferRef ,
PreambleBounds Bounds,
-   llvm::vfs::FileSystem *VFS) const {
+   llvm::vfs::FileSystem ) const {
 
   assert(
-  Bounds.Size <= MainFileBuffer->getBufferSize() &&
+  Bounds.Size <= MainFileBuffer.getBufferSize() &&
   "Buffer is too large. Bounds were calculated from a different buffer?");
 
   auto PreambleInvocation = std::make_shared(Invocation);
@@ -520,7 +520,7 @@
   if (PreambleBytes.size() != Bounds.Size ||
   PreambleEndsAtStartOfLine != Bounds.PreambleEndsAtStartOfLine ||
   !std::equal(PreambleBytes.begin(), PreambleBytes.end(),
-  MainFileBuffer->getBuffer().begin()))
+  MainFileBuffer.getBuffer().begin()))
 return false;
   // The preamble has not changed. We may be able to re-use the precompiled
   // preamble.
@@ -532,14 +532,14 @@
   llvm::StringSet<> OverriddenAbsPaths; // Either by buffers or files.
   for (const auto  : PreprocessorOpts.RemappedFiles) {
 llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(R.second), Status)) {
+if (!moveOnNoError(VFS.status(R.second), Status)) {
   // If we can't stat the file we're remapping to, assume that something
   // horrible happened.
   return false;
 }
 // If a mapped file was previously missing, then it has changed.
 llvm::SmallString<128> MappedPath(R.first);
-if (!VFS->makeAbsolute(MappedPath))
+if (!VFS.makeAbsolute(MappedPath))
   OverriddenAbsPaths.insert(MappedPath);
 
 OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
@@ -552,13 +552,13 @@
 const PrecompiledPreamble::PreambleFileHash PreambleHash =
 PreambleFileHash::createForMemoryBuffer(RB.second->getMemBufferRef());
 llvm::vfs::Status Status;
-if (moveOnNoError(VFS->status(RB.first), Status))
+if (moveOnNoError(VFS.status(RB.first), Status))
   OverriddenFiles[Status.getUniqueID()] = PreambleHash;
 else
   OverridenFileBuffers[RB.first] = PreambleHash;
 
 llvm::SmallString<128> MappedPath(RB.first);
-if (!VFS->makeAbsolute(MappedPath))
+if (!VFS.makeAbsolute(MappedPath))
   OverriddenAbsPaths.insert(MappedPath);
   }
 
@@ -574,7 +574,7 @@
 }
 
 llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(F.first()), Status)) {
+if (!moveOnNoError(VFS.status(F.first()), Status)) {
   // If the file's buffer is not remapped and we can't stat it,
   // assume that something horrible happened.
   return false;
@@ -603,7 +603,7 @@
   return false;
 // If a file previously recorded as missing exists as a regular file, then
 // consider the preamble out-of-date.
-if (auto Status = VFS->status(F.getKey())) {
+if (auto Status = VFS.status(F.getKey())) {
   if (Status->isRegularFile())
 return false;
 }
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1317,8 +1317,8 @@
 return nullptr;
 
   if (Preamble) {
-if (Preamble->CanReuse(PreambleInvocationIn, MainFileBuffer.get(), Bounds,
-   VFS.get())) {
+if (Preamble->CanReuse(PreambleInvocationIn, *MainFileBuffer, Bounds,
+   *VFS)) {
   // Okay! We can re-use the precompiled preamble.
 
   // Set the state of the diagnostic object to mimic its state
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -105,8 +105,8 @@
   /// Check whether PrecompiledPreamble can be reused for the new contents(\p
   /// MainFileBuffer) of the main file.
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer *MainFileBuffer, 

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Frontend/PrecompiledPreamble.h:108
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds 
Bounds,
-llvm::vfs::FileSystem *VFS) const;
+const llvm::MemoryBufferRef ,
+PreambleBounds Bounds, llvm::vfs::FileSystem ) const;

dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > kadircet wrote:
> > > > why not accept a value directly here? (i.e. drop const and ref)
> > > Ah, yes, I've done this a few times, and it still seems not quite right. 
> > > But the alternative also doesn't feel right when it's not necessary:
> > > ```
> > > #include "llvm/Basic/MemoryBufferRef.h"
> > > ```
> > > I'm happy either way since that file is fairly careful to avoid bloating 
> > > includes.
> > I agree this looks a bit odd, but avoiding an unnecessary include seems 
> > like a good excuse.
> @kadircet , WDYT?
sorry i was on vacation and just got the chance to get back to this.

I don't feel so bad about the include, as the header itself is small-ish and 
only includes StringRef.h, which is already included by this header. So I would 
lean towards accepting this by value and keeping the API clean, rather than 
trying to get away with a forward declaration.

but definitely up to you, i don't feel strongly about it in any case. (as you 
can easily make the argument of header for MemoryBufferRef getting bloated over 
time)


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a reviewer: kadircet.
dexonsmith added a comment.
This revision now requires review to proceed.

@kadircet, adding you explicitly as a blocking reviewer, since I suspect you've 
missed any emails generated from my previous pings. Please let me know whether 
you still have concerns.


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

@kadircet, ping, do you still have concerns?


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/include/clang/Frontend/PrecompiledPreamble.h:108
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds 
Bounds,
-llvm::vfs::FileSystem *VFS) const;
+const llvm::MemoryBufferRef ,
+PreambleBounds Bounds, llvm::vfs::FileSystem ) const;

jansvoboda11 wrote:
> dexonsmith wrote:
> > kadircet wrote:
> > > why not accept a value directly here? (i.e. drop const and ref)
> > Ah, yes, I've done this a few times, and it still seems not quite right. 
> > But the alternative also doesn't feel right when it's not necessary:
> > ```
> > #include "llvm/Basic/MemoryBufferRef.h"
> > ```
> > I'm happy either way since that file is fairly careful to avoid bloating 
> > includes.
> I agree this looks a bit odd, but avoiding an unnecessary include seems like 
> a good excuse.
@kadircet , WDYT?


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 accepted this revision.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/include/clang/Frontend/PrecompiledPreamble.h:108
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds 
Bounds,
-llvm::vfs::FileSystem *VFS) const;
+const llvm::MemoryBufferRef ,
+PreambleBounds Bounds, llvm::vfs::FileSystem ) const;

dexonsmith wrote:
> kadircet wrote:
> > why not accept a value directly here? (i.e. drop const and ref)
> Ah, yes, I've done this a few times, and it still seems not quite right. But 
> the alternative also doesn't feel right when it's not necessary:
> ```
> #include "llvm/Basic/MemoryBufferRef.h"
> ```
> I'm happy either way since that file is fairly careful to avoid bloating 
> includes.
I agree this looks a bit odd, but avoiding an unnecessary include seems like a 
good excuse.


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/include/clang/Frontend/PrecompiledPreamble.h:108
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds 
Bounds,
-llvm::vfs::FileSystem *VFS) const;
+const llvm::MemoryBufferRef ,
+PreambleBounds Bounds, llvm::vfs::FileSystem ) const;

kadircet wrote:
> why not accept a value directly here? (i.e. drop const and ref)
Ah, yes, I've done this a few times, and it still seems not quite right. But 
the alternative also doesn't feel right when it's not necessary:
```
#include "llvm/Basic/MemoryBufferRef.h"
```
I'm happy either way since that file is fairly careful to avoid bloating 
includes.


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/include/clang/Frontend/PrecompiledPreamble.h:108
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds 
Bounds,
-llvm::vfs::FileSystem *VFS) const;
+const llvm::MemoryBufferRef ,
+PreambleBounds Bounds, llvm::vfs::FileSystem ) const;

why not accept a value directly here? (i.e. drop const and ref)


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

ping


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

https://reviews.llvm.org/D91297

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

[PATCH] D91297: Frontend: Take VFS and MainFileBuffer by reference in PrecompiledPreamble::CanReuse, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
dexonsmith added reviewers: JDevlieghere, arphaman, jansvoboda11.
Herald added subscribers: usaxena95, ributzka, kadircet.
dexonsmith requested review of this revision.

Clarify that `PrecompiledPreamble::CanReuse` requires non-null arguments
for `VFS` and `MainFileBuffer`, taking them by reference instead of by
pointer.


https://reviews.llvm.org/D91297

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp

Index: clang/lib/Frontend/PrecompiledPreamble.cpp
===
--- clang/lib/Frontend/PrecompiledPreamble.cpp
+++ clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -501,12 +501,12 @@
 }
 
 bool PrecompiledPreamble::CanReuse(const CompilerInvocation ,
-   const llvm::MemoryBuffer *MainFileBuffer,
+   const llvm::MemoryBufferRef ,
PreambleBounds Bounds,
-   llvm::vfs::FileSystem *VFS) const {
+   llvm::vfs::FileSystem ) const {
 
   assert(
-  Bounds.Size <= MainFileBuffer->getBufferSize() &&
+  Bounds.Size <= MainFileBuffer.getBufferSize() &&
   "Buffer is too large. Bounds were calculated from a different buffer?");
 
   auto PreambleInvocation = std::make_shared(Invocation);
@@ -520,7 +520,7 @@
   if (PreambleBytes.size() != Bounds.Size ||
   PreambleEndsAtStartOfLine != Bounds.PreambleEndsAtStartOfLine ||
   !std::equal(PreambleBytes.begin(), PreambleBytes.end(),
-  MainFileBuffer->getBuffer().begin()))
+  MainFileBuffer.getBuffer().begin()))
 return false;
   // The preamble has not changed. We may be able to re-use the precompiled
   // preamble.
@@ -532,14 +532,14 @@
   llvm::StringSet<> OverriddenAbsPaths; // Either by buffers or files.
   for (const auto  : PreprocessorOpts.RemappedFiles) {
 llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(R.second), Status)) {
+if (!moveOnNoError(VFS.status(R.second), Status)) {
   // If we can't stat the file we're remapping to, assume that something
   // horrible happened.
   return false;
 }
 // If a mapped file was previously missing, then it has changed.
 llvm::SmallString<128> MappedPath(R.first);
-if (!VFS->makeAbsolute(MappedPath))
+if (!VFS.makeAbsolute(MappedPath))
   OverriddenAbsPaths.insert(MappedPath);
 
 OverriddenFiles[Status.getUniqueID()] = PreambleFileHash::createForFile(
@@ -552,13 +552,13 @@
 const PrecompiledPreamble::PreambleFileHash PreambleHash =
 PreambleFileHash::createForMemoryBuffer(RB.second->getMemBufferRef());
 llvm::vfs::Status Status;
-if (moveOnNoError(VFS->status(RB.first), Status))
+if (moveOnNoError(VFS.status(RB.first), Status))
   OverriddenFiles[Status.getUniqueID()] = PreambleHash;
 else
   OverridenFileBuffers[RB.first] = PreambleHash;
 
 llvm::SmallString<128> MappedPath(RB.first);
-if (!VFS->makeAbsolute(MappedPath))
+if (!VFS.makeAbsolute(MappedPath))
   OverriddenAbsPaths.insert(MappedPath);
   }
 
@@ -574,7 +574,7 @@
 }
 
 llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(F.first()), Status)) {
+if (!moveOnNoError(VFS.status(F.first()), Status)) {
   // If the file's buffer is not remapped and we can't stat it,
   // assume that something horrible happened.
   return false;
@@ -603,7 +603,7 @@
   return false;
 // If a file previously recorded as missing exists as a regular file, then
 // consider the preamble out-of-date.
-if (auto Status = VFS->status(F.getKey())) {
+if (auto Status = VFS.status(F.getKey())) {
   if (Status->isRegularFile())
 return false;
 }
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1316,8 +1316,8 @@
 return nullptr;
 
   if (Preamble) {
-if (Preamble->CanReuse(PreambleInvocationIn, MainFileBuffer.get(), Bounds,
-   VFS.get())) {
+if (Preamble->CanReuse(PreambleInvocationIn, *MainFileBuffer, Bounds,
+   *VFS)) {
   // Okay! We can re-use the precompiled preamble.
 
   // Set the state of the diagnostic object to mimic its state
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -105,8 +105,8 @@
   /// Check whether PrecompiledPreamble can be reused for the new contents(\p
   /// MainFileBuffer) of the main file.
   bool CanReuse(const CompilerInvocation ,
-const llvm::MemoryBuffer