[PATCH] D38578: [preamble] Also record the "skipping" state of the preprocessor

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

@nik, pretty sure this fix  I submitted ages 
ago fixes that. Been using it in production a couple years. Would be nice if 
someone reviewed it so we could finally upstream it.


https://reviews.llvm.org/D38578



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

@yvvan: The clang frontend tests (`PCHPreambleTest` and friends) are disabled 
on Windows in the makefile (I think because the VFS tests depend on linux-like 
paths). So running the tests on Windows without failures is encouraging but not 
the whole story.


Repository:
  rC Clang

https://reviews.llvm.org/D41005



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Ping?
The patch is ready to go, just needs a final approval...


https://reviews.llvm.org/D20124



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

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

The `IsValid = true` line coincidentally fixes a tangentially related bug -- 
see https://reviews.llvm.org/D20338 (in which I tried to introduce the same fix 
almost a year earlier, but nobody accepted the review). I guess I have to 
maintain the test for that case out of tree, though I could try to upstream it 
again (however, given that it's now passing with your change, I don't have much 
hope of it being accepted).


https://reviews.llvm.org/D27810



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

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 created this revision.
Herald added a subscriber: mgorny.

When using a virtual file-system (VFS) and a preamble file (PCH) is generated, 
it is generated on-disk in the real file-system instead of in the VFS (which I 
guess makes sense, since the VFS is read-only). However, when subsequently 
reading the generated PCH, the frontend passes through the VFS it has been 
given -- resulting in an error and a failed parse (since the VFS doesn't 
contain the PCH; the real filesystem does).

This patch fixes that by detecting when a VFS is being used for a parse that 
needs to work with a PCH file, and creating an overlay VFS that includes the 
real file-system underneath. Since the PCH is hard-coded to always be on the 
real file-system, I believe this is the cleanest approach.

This allows tests to be written which make use of both PCH files and a VFS, 
like the one I've included here.

Note: This was originally part of the code to test the bug fixed in 
https://reviews.llvm.org/D20338, but while languishing in review it has since 
been fixed by somebody else in https://reviews.llvm.org/D27810. However, I feel 
it's still important to be able to test the frontend preamble code while at the 
same time making use of a VFS, so I've rebased that part of the patch (and my 
test to go with it).


https://reviews.llvm.org/D37474

Files:
  lib/Frontend/ASTUnit.cpp
  unittests/Frontend/CMakeLists.txt
  unittests/Frontend/PchPreambleTest.cpp

Index: unittests/Frontend/PchPreambleTest.cpp
===
--- /dev/null
+++ unittests/Frontend/PchPreambleTest.cpp
@@ -0,0 +1,156 @@
+//-- unittests/Frontend/PchPreambleTest.cpp - FrontendAction tests ---//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+  std::map ReadCounts;
+
+public:
+  ErrorOr> openFileForRead(const Twine &Path) override
+  {
+SmallVector PathVec;
+Path.toVector(PathVec);
+llvm::sys::path::remove_dots(PathVec, true);
+++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+return InMemoryFileSystem::openFileForRead(Path);
+  }
+
+  unsigned GetReadCount(const Twine &Path) const
+  {
+auto it = ReadCounts.find(Path.str());
+return it == ReadCounts.end() ? 0 : it->second;
+  }
+};
+
+class PchPreambleTest : public ::testing::Test {
+  IntrusiveRefCntPtr VFS;
+  StringMap RemappedFiles;
+  std::shared_ptr PCHContainerOpts;
+  FileSystemOptions FSOpts;
+
+public:
+  void SetUp() override {
+VFS = new ReadCountingInMemoryFileSystem();
+// We need the working directory to be set to something absolute,
+// otherwise it ends up being inadvertently set to the current
+// working directory in the real file system due to a series of
+// unfortunate conditions interacting badly.
+// What's more, this path *must* be absolute on all (real)
+// filesystems, so just '/' won't work (e.g. on Win32).
+VFS->setCurrentWorkingDirectory("//./");
+  }
+
+  void TearDown() override {
+  }
+
+  void AddFile(const std::string &Filename, const std::string &Contents) {
+::time_t now;
+::time(&now);
+VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
+  }
+
+  void RemapFile(const std::string &Filename, const std::string &Contents) {
+RemappedFiles[Filename] = Contents;
+  }
+
+  std::unique_ptr ParseAST(const std::string &EntryFile) {
+PCHContainerOpts = std::make_shared();
+std::shared_ptr CI(new CompilerInvocation);
+CI->getFrontendOpts().Inputs.push_back(
+  FrontendInputFile(EntryFile, FrontendOptions::getInputKindForExtension(
+llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
+
+PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
+PPOpts.RemappedFilesKeepOriginalName = true;
+
+IntrusiveRefCntPtr
+  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompile

[PATCH] D20338: [PCH] Fixed overridden files always invalidating preamble even when unchanged

[Cameron via Phabricator via cfe-commits]

cameron314 abandoned this revision.
cameron314 added a comment.

This patch is obsolete. While waiting over a year for a review, somebody else 
came across the same fix (for a different manifestation of the same bug) in 
https://reviews.llvm.org/D27810 and managed to get it through. I think my test 
case and supporting changes are still useful, however, so I've submitted that 
separately in https://reviews.llvm.org/D37474.


https://reviews.llvm.org/D20338



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

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

I suppose we could do the overlay manually in all the tests that need it; I 
guess it depends on what the goal of the VFS support is. To my mind, it doesn't 
make sense that a file is placed in the RealFS in the first place if a VFS is 
used, but this is quite entrenched in the current design and difficult to 
change. In which case, I would argue that parsing a file with a preamble and a 
VFS should not give a cryptic error without an explicit overlay, but rather 
"just work". But I agree my current patch is a little crude in that it allows 
access to the entire RealFS via the VFS.

I could change the automatic overlay to *only* support access to the generated 
PCH file, similar to how the `FilteredFileSystem` you've pointed out works, but 
for a single file only. Would that be acceptable?


https://reviews.llvm.org/D37474



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

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 created this revision.

This patch fixes preamble skipping when the preamble region includes a byte 
order mark (BOM). Previously, parsing would fail if preamble PCH generation was 
enabled and a BOM was present.

This also fixes the preamble incorrectly being invalidated when a BOM appeared 
or disappeared, even if the contents after the BOM had not changed. This may 
seem to be an obscure edge case, but it is important for IDEs that pass buffer 
overrides that never (or always) have a BOM, yet the underlying file from the 
initial parse that generated a PCH might (or might not) have a BOM.

Test cases for these scenarios are included.

Note: This depends on the test infrastructure introduced in 
https://reviews.llvm.org/D37474.


https://reviews.llvm.org/D37491

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  include/clang/Lex/Lexer.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp
  lib/Lex/Lexer.cpp
  lib/Lex/Preprocessor.cpp
  unittests/Frontend/PchPreambleTest.cpp

Index: unittests/Frontend/PchPreambleTest.cpp
===
--- unittests/Frontend/PchPreambleTest.cpp
+++ unittests/Frontend/PchPreambleTest.cpp
@@ -91,8 +91,19 @@
 PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
 PPOpts.RemappedFilesKeepOriginalName = true;
 
+struct DebugConsumer : DiagnosticConsumer
+{
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const Diagnostic &Info) override {
+DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
+SmallString<16> DiagText;
+Info.FormatDiagnostic(DiagText);
+llvm::errs() << DiagText << '\n';
+  }
+};
+
 IntrusiveRefCntPtr
-  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DebugConsumer));
 
 FileManager *FileMgr = new FileManager(FSOpts, VFS);
 
@@ -153,4 +164,64 @@
   ASSERT_EQ(initialCounts[2], GetFileReadCount(Header2));
 }
 
+TEST_F(PchPreambleTest, ReparseWithDisappearingBom) {
+  std::string Header = "//./header.h";
+  std::string Main = "//./main.cpp";
+  AddFile(Header, "int random() { return 4; }");
+  AddFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  std::unique_ptr AST(ParseAST(Main));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  unsigned initialCounts[] = {
+GetFileReadCount(Main),
+GetFileReadCount(Header)
+  };
+
+  // Remove BOM but keep the rest the same
+  RemapFile(Main,
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_EQ(initialCounts[0], GetFileReadCount(Main));
+  ASSERT_EQ(initialCounts[1], GetFileReadCount(Header));
+}
+
+TEST_F(PchPreambleTest, ReparseWithAppearingBom) {
+  std::string Header = "//./header.h";
+  std::string Main = "//./main.cpp";
+  AddFile(Header, "int random() { return 4; }");
+  AddFile(Main,
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  std::unique_ptr AST(ParseAST(Main));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  unsigned initialCounts[] = {
+GetFileReadCount(Main),
+GetFileReadCount(Header)
+  };
+
+  // Suddenly, a BOM appeared
+  RemapFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_EQ(initialCounts[0], GetFileReadCount(Main));
+  ASSERT_EQ(initialCounts[1], GetFileReadCount(Header));
+}
+
 } // anonymous namespace
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -516,9 +516,9 @@
 // If we've been asked to skip bytes in the main file (e.g., as part of a
 // precompiled preamble), do so now.
 if (SkipMainFilePreamble.first > 0)
-  CurLexer->SkipBytes(SkipMainFilePreamble.first, 
-  SkipMainFilePreamble.second);
-
+  CurLexer->SetByteOffset(SkipMainFilePreamble.first,
+  SkipMainFilePreamble.second);
+
 // Tell the header info that the main file was entered.  If the file is later
 // #imported, it won't be re-entered.
 if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID))
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -552,9 +552,9 @@
 
 } // end anonymous namespace
 
-std::pair Lexer::ComputePreamble(StringRef Buffer,
-   

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Thanks for the response!

> How are various preprocessor offests (and SourceLocation offsets) are 
> calculated? Do they account for BOM presence and ignore it?

Everything is in byte offsets; the `SourceLocation` after the BOM is not the 
same as before the BOM. The lexer automatically skips the BOM at the beginning 
of the file if it sees one (`Lexer::InitLexer`), and everything else works 
normally after that. The start of the first line is after the BOM, if any, 
which means it doesn't affect line/column numbers.

> Are there potential problems we may run into because of the changing offsets? 
> Could we add tests checking changing the offsets does not matter?

That's a good point; I've looked into it and the PCH for the preamble is parsed 
using just the buffer slice that contains the preamble, excluding any BOM. That 
means that when we resume parsing later on a main buffer with a BOM, the 
`SourceLocation`s within the preamble itself will be off. However, normally 
this doesn't matter since the only things in the preamble are preprocessor 
directives, whose positions are very rarely used. (I should note at this point 
that we've been using a variant of this patch in production for a few years 
without any problem.) So, we have two choices: Either parse the preamble with 
the BOM and throw out the preamble/PCH when the BOM presence changes from the 
main buffer, or slice the buffer when using a preamble PCH so that it never has 
a BOM during parsing. I'm leaning towards the second option, since it's a 
little cleaner and lets the preamble be reused more easily; the only downside 
is that an external consumer would not be able to use any absolute offsets from 
the AST (note that line/column offsets would be identical) in the original 
buffer if it has a BOM -- but in any case, absolute offsets are usually useless 
without the buffer itself, which if obtained from clang would always be the 
correct buffer.

> Should we add checks that BOM was removed or added, but not changed? I would 
> not expect preamble to be reusable "as is" if BOM (and therefore, input 
> encoding) changed.

I'm not sure I understand this point. Clang only understands UTF-8; the BOM is 
either present or not, but the encoding never changes. (And the BOM itself is 
always the same byte sequence too.) It has no impact on the file contents.




Comment at: include/clang/Frontend/PrecompiledPreamble.h:102
+  /// appears or disappears between parses).
+  void UpdateOffset(unsigned NewOffset);
+

ilya-biryukov wrote:
> Let's leave this class's interface immutable. It is used concurrently in 
> clangd and having a mutable method like this would break the code.
> 
> Passing new `PreambleBounds` to `AddImplicitPreamble` and setting the offsets 
> accordingly would do the trick, leave the interface immutable and make the 
> fact that offsets might change more evident.
Fair point, I'll change this.



Comment at: include/clang/Frontend/PrecompiledPreamble.h:191
   bool PreambleEndsAtStartOfLine;
+  /// The start offset of the bounds used to build the preamble.
+  unsigned PreambleOffset;

ilya-biryukov wrote:
> Let's store original `PreambleBounds` instead of `PreambleEndsAtStartOfLine` 
> and `PreambleOffset`.
> It would make the code easier to read.
Again, good point, I'll change this.



Comment at: include/clang/Lex/Lexer.h:50
+  /// \brief Start offset of the preamble, in bytes.
+  unsigned StartOffset;
+  /// \brief Size of the preamble in bytes.

ilya-biryukov wrote:
> Maybe pick a name that clearly states that it's a `BOM` size? 
> Or add a comment indicating that it's a `BOM` offset.
I can see how this might be confusing. I'll add a comment.



Comment at: include/clang/Lex/Lexer.h:639
 
-  void SkipBytes(unsigned Bytes, bool StartOfLine);
+  void SetByteOffset(unsigned Offset, bool StartOfLine);
 

ilya-biryukov wrote:
> Maybe leave the old name? Doesn't `SkipBytes` captures the new semantics just 
> as good?
`SkipBytes` moves relative to the current position, but the lexer skips the BOM 
implicitly on construction; I don't want to skip it twice. `SetByteOffset` is 
absolute, which makes it simple and clear to use without having to reason about 
implicit past state.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:195
 
 PreambleBounds clang::ComputePreambleBounds(const LangOptions &LangOpts,
 llvm::MemoryBuffer *Buffer,

ilya-biryukov wrote:
> Could you inline usages of this function and remove it?
> 
I could; I think it makes sense to leave the wrapper, though, since the 
`ASTUnit` deals with the `PrecompiledPreamble` at its level of abstraction, and 
the `PrecompiledPreamble` deals with the lexer at its level of abstraction.



Comment at: unittests/Frontend/PchPreambleTest.cpp:19

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

I'll change the overlay to only allow access to the PCH.

I agree that it would be nice to only read the PCH using real filesystem APIs 
in order to be symmetrical, but this seems non-trivial. It also feels like a 
step in the wrong direction -- ideally the VFS would hold the PCH too instead 
of putting it on disk, in my opinion.

I'm not sure adding a remapped file buffer would work, since it's unclear to me 
exactly at which point the PCH is written relative to the multiple places it's 
read. Additionally, there's code that  does directory traversal in the current 
VFS to find a PCH when given a directory name instead of a file name; this only 
works if the VFS is aware of the PCH, or if the code is rewritten to use the 
underlying file system.


https://reviews.llvm.org/D37474



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

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 114016.
cameron314 added a comment.

Here's an updated patch that allows only the PCH to be accessed from the real 
FS when a VSF is present. Tests still pass.


https://reviews.llvm.org/D37474

Files:
  lib/Frontend/ASTUnit.cpp
  unittests/Frontend/CMakeLists.txt
  unittests/Frontend/PchPreambleTest.cpp

Index: unittests/Frontend/PchPreambleTest.cpp
===
--- /dev/null
+++ unittests/Frontend/PchPreambleTest.cpp
@@ -0,0 +1,156 @@
+//-- unittests/Frontend/PchPreambleTest.cpp - FrontendAction tests ---//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+  std::map ReadCounts;
+
+public:
+  ErrorOr> openFileForRead(const Twine &Path) override
+  {
+SmallVector PathVec;
+Path.toVector(PathVec);
+llvm::sys::path::remove_dots(PathVec, true);
+++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+return InMemoryFileSystem::openFileForRead(Path);
+  }
+
+  unsigned GetReadCount(const Twine &Path) const
+  {
+auto it = ReadCounts.find(Path.str());
+return it == ReadCounts.end() ? 0 : it->second;
+  }
+};
+
+class PchPreambleTest : public ::testing::Test {
+  IntrusiveRefCntPtr VFS;
+  StringMap RemappedFiles;
+  std::shared_ptr PCHContainerOpts;
+  FileSystemOptions FSOpts;
+
+public:
+  void SetUp() override {
+VFS = new ReadCountingInMemoryFileSystem();
+// We need the working directory to be set to something absolute,
+// otherwise it ends up being inadvertently set to the current
+// working directory in the real file system due to a series of
+// unfortunate conditions interacting badly.
+// What's more, this path *must* be absolute on all (real)
+// filesystems, so just '/' won't work (e.g. on Win32).
+VFS->setCurrentWorkingDirectory("//./");
+  }
+
+  void TearDown() override {
+  }
+
+  void AddFile(const std::string &Filename, const std::string &Contents) {
+::time_t now;
+::time(&now);
+VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
+  }
+
+  void RemapFile(const std::string &Filename, const std::string &Contents) {
+RemappedFiles[Filename] = Contents;
+  }
+
+  std::unique_ptr ParseAST(const std::string &EntryFile) {
+PCHContainerOpts = std::make_shared();
+std::shared_ptr CI(new CompilerInvocation);
+CI->getFrontendOpts().Inputs.push_back(
+  FrontendInputFile(EntryFile, FrontendOptions::getInputKindForExtension(
+llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
+
+PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
+PPOpts.RemappedFilesKeepOriginalName = true;
+
+IntrusiveRefCntPtr
+  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
+  CI, PCHContainerOpts, Diags, FileMgr, false, false,
+  /*PrecompilePreambleAfterNParses=*/1);
+return AST;
+  }
+
+  bool ReparseAST(const std::unique_ptr &AST) {
+bool reparseFailed = AST->Reparse(PCHContainerOpts, GetRemappedFiles(), VFS);
+return !reparseFailed;
+  }
+
+  unsigned GetFileReadCount(const std::string &Filename) const {
+return VFS->GetReadCount(Filename);
+  }
+
+private:
+  std::vector>
+  GetRemappedFiles() const {
+std::vector> Remapped;
+for (const auto &RemappedFile : RemappedFiles) {
+  std::unique_ptr buf = MemoryBuffer::getMemBufferCopy(
+RemappedFile.second, RemappedFile.first());
+  Remapped.emplace_back(RemappedFile.first(), buf.release());
+}
+return Remapped;
+  }
+};
+
+TEST_F(PchPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
+  std::string Header1 = "//./header1.h";
+  std::string Header2 = "//./header2.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(Header1, "");
+  AddFile(Header2, "#pragma once");
+  AddFile(MainName,
+"#include \"//./foo/../header1.h\"\n"
+ 

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Looking at the way remapped buffers are handled, I just remembered that they 
must exist on the file system (at the very least, in a directory that exists) 
or the remapping is not taken into account. So that pretty much rules out the 
other approach, I think.


https://reviews.llvm.org/D37474



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

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

> Maybe there's a third option option to remove the BOM from the buffer before 
> passing it to clang?
>  Could you elaborate on your use-case a little more? Is there no way to 
> consistently always pass buffers either with or without BOM?
>  Out of two options you mention discarding preamble on BOM changes seems like 
> an easy option that is both correct and won't make a difference in 
> performance since BOM rarely changes.
>  Looking at your use-case, it sounds like you'll only have 1 extra reparse of 
> preamble, which is probably fine.

In my particular use case, when the file is remapped by the IDE, there's never 
a BOM. But when it's not remapped, the real file may or may not have a BOM. 
Since the file goes back and forth between mapped and unmapped depending on 
whether it's saved, the BOM presence can change quite frequently, and we don't 
really have control over it (the BOM can change on disk too). This is a common 
use case for anyone integrating clang/libclang into an IDE; the rarity of UTF-8 
BOMs on platforms other than Windows probably obscured this until now.

I think since we can handle the changing BOM presence in the preamble 
gracefully, we should. I'll draft a patch that does the slicing correctly so 
that the offsets are always valid.

> Sure, it's not something clang supports, it's an edge-case when clang 
> receives "malformed" input. Does lexer only skip utf-8 BOM, but not other 
> versions of BOM?
>  But you're right, it's highly unlikely anything will break in that case.

Ah, I see. Yes, the lexer only skips a UTF-8 BOM, but I seem to recall seeing 
some code that detects BOMs in other encodings and emits an error (in the 
driver, possibly?).




Comment at: unittests/Frontend/PchPreambleTest.cpp:190
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());

ilya-biryukov wrote:
> cameron314 wrote:
> > ilya-biryukov wrote:
> > > We're not really testing that preamble was reused.
> > > Maybe return a flag from `ASTUnit::Reparse` to indicate if preamble was 
> > > reused and check it here?
> > We are; if it wasn't reused, the header would have been opened again and 
> > the last assert on `GetFileReadCount` below would fail.
> Missed that, thanks. Looks good.
> Maybe add a comment explicitly noting that?
Sure, will do.


https://reviews.llvm.org/D37491



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

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 114228.
cameron314 added a comment.

Here's an updated patch. The code required to make it work is much simpler when 
the BOM is simply ignored :-)


https://reviews.llvm.org/D37491

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  include/clang/Lex/Lexer.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp
  lib/Lex/Lexer.cpp
  lib/Lex/Preprocessor.cpp
  unittests/Frontend/PchPreambleTest.cpp

Index: unittests/Frontend/PchPreambleTest.cpp
===
--- unittests/Frontend/PchPreambleTest.cpp
+++ unittests/Frontend/PchPreambleTest.cpp
@@ -91,8 +91,19 @@
 PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
 PPOpts.RemappedFilesKeepOriginalName = true;
 
+struct DebugConsumer : DiagnosticConsumer
+{
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const Diagnostic &Info) override {
+DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
+SmallString<16> DiagText;
+Info.FormatDiagnostic(DiagText);
+llvm::errs() << DiagText << '\n';
+  }
+};
+
 IntrusiveRefCntPtr
-  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DebugConsumer));
 
 FileManager *FileMgr = new FileManager(FSOpts, VFS);
 
@@ -153,4 +164,70 @@
   ASSERT_EQ(initialCounts[2], GetFileReadCount(Header2));
 }
 
+TEST_F(PchPreambleTest, ReparseWithDisappearingBom) {
+  std::string Header = "//./header.h";
+  std::string Main = "//./main.cpp";
+  AddFile(Header, "int random() { return 4; }");
+  AddFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  std::unique_ptr AST(ParseAST(Main));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Check the file counts to make sure the preamble is
+  // really being used (and the header isn't being read
+  // again)
+  unsigned initialCounts[] = {
+GetFileReadCount(Main),
+GetFileReadCount(Header)
+  };
+
+  // Remove BOM but keep the rest the same
+  RemapFile(Main,
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_EQ(initialCounts[0], GetFileReadCount(Main));
+  ASSERT_EQ(initialCounts[1], GetFileReadCount(Header));
+}
+
+TEST_F(PchPreambleTest, ReparseWithAppearingBom) {
+  std::string Header = "//./header.h";
+  std::string Main = "//./main.cpp";
+  AddFile(Header, "int random() { return 4; }");
+  AddFile(Main,
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  std::unique_ptr AST(ParseAST(Main));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Check the file counts to make sure the preamble is
+  // really being used (and the header isn't being read
+  // again)
+  unsigned initialCounts[] = {
+GetFileReadCount(Main),
+GetFileReadCount(Header)
+  };
+
+  // Suddenly, a BOM appeared
+  RemapFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() * 2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_EQ(initialCounts[0], GetFileReadCount(Main));
+  ASSERT_EQ(initialCounts[1], GetFileReadCount(Header));
+}
+
 } // anonymous namespace
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -516,9 +516,9 @@
 // If we've been asked to skip bytes in the main file (e.g., as part of a
 // precompiled preamble), do so now.
 if (SkipMainFilePreamble.first > 0)
-  CurLexer->SkipBytes(SkipMainFilePreamble.first, 
+  CurLexer->SkipBytes(SkipMainFilePreamble.first,
   SkipMainFilePreamble.second);
-
+
 // Tell the header info that the main file was entered.  If the file is later
 // #imported, it won't be re-entered.
 if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID))
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -552,9 +552,9 @@
 
 } // end anonymous namespace
 
-std::pair Lexer::ComputePreamble(StringRef Buffer,
- const LangOptions &LangOpts,
- unsigned MaxLines) {
+PreambleBounds Lexer::ComputePreamble(StringRef Buffer,
+  const LangOptions &LangOpts,
+  unsigned MaxLines) {
   // Create a lexer starting at the beginning of the file. Note that 

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 added inline comments.



Comment at: lib/Frontend/ASTUnit.cpp:1014
+/// with another virtual file system.
+class PCHOverlayFileSystem : public vfs::FileSystem
+{

ilya-biryukov wrote:
> Maybe create a combination of `InMemoryFileSystem` and `OverlayFileSystem` 
> instead of custom filtering implementation?
> We really need to read only a single file given that `ASTUnit` never creates 
> directory PCHs.
> I bet it would make the code simpler and less error-prone.
I hadn't thought of that. Yes, that makes sense and is more concise.



Comment at: lib/Frontend/ASTUnit.cpp:1090
+  IntrusiveRefCntPtr RealFS = vfs::getRealFileSystem();
+  if (OverrideMainBuffer && VFS && RealFS && VFS != RealFS) {
+// We have a slight inconsistency here -- we're using the VFS to

ilya-biryukov wrote:
> Maybe create a PCH overlay only when `!VFS->exists(/*PreamblePath...*/)`?
> This seems like a good enough indication that `RealFS` is underneath the 
> `vfs::FileSystem` instance, even if pointer equality does not hold (i.e. in 
> case of overlays over `RealFS`).
Makes sense.



Comment at: unittests/Frontend/CMakeLists.txt:9
   CodeGenActionTest.cpp
+  PchPreambleTest.cpp
   )

ilya-biryukov wrote:
> Maybe rename to `PCHPreambleTest`?
> LLVM code generally capitalizes all letters in abbreviations.
Oops, overlooked this one. Will do!


https://reviews.llvm.org/D37474



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

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 114414.
cameron314 added a comment.

The latest patch. I think this one should do the trick :-)


https://reviews.llvm.org/D37474

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  unittests/Frontend/CMakeLists.txt
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- /dev/null
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -0,0 +1,156 @@
+//-- unittests/Frontend/PCHPreambleTest.cpp - FrontendAction tests ---//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+  std::map ReadCounts;
+
+public:
+  ErrorOr> openFileForRead(const Twine &Path) override
+  {
+SmallVector PathVec;
+Path.toVector(PathVec);
+llvm::sys::path::remove_dots(PathVec, true);
+++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+return InMemoryFileSystem::openFileForRead(Path);
+  }
+
+  unsigned GetReadCount(const Twine &Path) const
+  {
+auto it = ReadCounts.find(Path.str());
+return it == ReadCounts.end() ? 0 : it->second;
+  }
+};
+
+class PCHPreambleTest : public ::testing::Test {
+  IntrusiveRefCntPtr VFS;
+  StringMap RemappedFiles;
+  std::shared_ptr PCHContainerOpts;
+  FileSystemOptions FSOpts;
+
+public:
+  void SetUp() override {
+VFS = new ReadCountingInMemoryFileSystem();
+// We need the working directory to be set to something absolute,
+// otherwise it ends up being inadvertently set to the current
+// working directory in the real file system due to a series of
+// unfortunate conditions interacting badly.
+// What's more, this path *must* be absolute on all (real)
+// filesystems, so just '/' won't work (e.g. on Win32).
+VFS->setCurrentWorkingDirectory("//./");
+  }
+
+  void TearDown() override {
+  }
+
+  void AddFile(const std::string &Filename, const std::string &Contents) {
+::time_t now;
+::time(&now);
+VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
+  }
+
+  void RemapFile(const std::string &Filename, const std::string &Contents) {
+RemappedFiles[Filename] = Contents;
+  }
+
+  std::unique_ptr ParseAST(const std::string &EntryFile) {
+PCHContainerOpts = std::make_shared();
+std::shared_ptr CI(new CompilerInvocation);
+CI->getFrontendOpts().Inputs.push_back(
+  FrontendInputFile(EntryFile, FrontendOptions::getInputKindForExtension(
+llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
+
+PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
+PPOpts.RemappedFilesKeepOriginalName = true;
+
+IntrusiveRefCntPtr
+  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
+  CI, PCHContainerOpts, Diags, FileMgr, false, false,
+  /*PrecompilePreambleAfterNParses=*/1);
+return AST;
+  }
+
+  bool ReparseAST(const std::unique_ptr &AST) {
+bool reparseFailed = AST->Reparse(PCHContainerOpts, GetRemappedFiles(), VFS);
+return !reparseFailed;
+  }
+
+  unsigned GetFileReadCount(const std::string &Filename) const {
+return VFS->GetReadCount(Filename);
+  }
+
+private:
+  std::vector>
+  GetRemappedFiles() const {
+std::vector> Remapped;
+for (const auto &RemappedFile : RemappedFiles) {
+  std::unique_ptr buf = MemoryBuffer::getMemBufferCopy(
+RemappedFile.second, RemappedFile.first());
+  Remapped.emplace_back(RemappedFile.first(), buf.release());
+}
+return Remapped;
+  }
+};
+
+TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
+  std::string Header1 = "//./header1.h";
+  std::string Header2 = "//./header2.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(Header1, "");
+  AddFile(Header2, "#pragma once");
+  AddFile(MainName,
+"#include \"//./foo/../header1.h\"\n"
+"#include \"//

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

It seems there's other users of `PrecompiledPreamble` that would have to be 
fixed, yes. If we go with my original fix of taking into account the BOM in the 
preamble bounds, there's no way of reusing the PCH when the BOM 
appears/disappears. I still maintain this is a common use case for IDE-type 
clients. This type of performance bug is very hard to track down.

@erikjv: Yes, I think this will fix PR25023.
PR21144 is unrelated; clang uses UTF-8 byte offsets instead of 
logical-character offsets for column numbers, which makes sense to me.




Comment at: lib/Frontend/ASTUnit.cpp:1262
+  // This ensures offsets within and past the preamble stay valid.
+  if (MainFileBuffer->getBuffer().startswith("\xEF\xBB\xBF")) {
+MainFileBuffer = llvm::MemoryBuffer::getMemBufferSlice(

ilya-biryukov wrote:
> It seems that having only this chunk would fix your issue.
> Everything else is just a non-functional refactoring, maybe let's focus on 
> that part (and tests) in this review and send the rest as a separate change?
> To keep refactoring and functional changes logically separate.
Yes, with this form of the fix, the other changes are mostly cosmetic. I could 
simply revert them, it's not worth the hassle of submitting another patch.


https://reviews.llvm.org/D37491



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

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 114604.
cameron314 added a comment.

Final diff, will commit soon. Thanks!


https://reviews.llvm.org/D37474

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  unittests/Frontend/CMakeLists.txt
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- /dev/null
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -0,0 +1,156 @@
+//-- unittests/Frontend/PCHPreambleTest.cpp - FrontendAction tests ---//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+  std::map ReadCounts;
+
+public:
+  ErrorOr> openFileForRead(const Twine &Path) override
+  {
+SmallVector PathVec;
+Path.toVector(PathVec);
+llvm::sys::path::remove_dots(PathVec, true);
+++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+return InMemoryFileSystem::openFileForRead(Path);
+  }
+
+  unsigned GetReadCount(const Twine &Path) const
+  {
+auto it = ReadCounts.find(Path.str());
+return it == ReadCounts.end() ? 0 : it->second;
+  }
+};
+
+class PCHPreambleTest : public ::testing::Test {
+  IntrusiveRefCntPtr VFS;
+  StringMap RemappedFiles;
+  std::shared_ptr PCHContainerOpts;
+  FileSystemOptions FSOpts;
+
+public:
+  void SetUp() override {
+VFS = new ReadCountingInMemoryFileSystem();
+// We need the working directory to be set to something absolute,
+// otherwise it ends up being inadvertently set to the current
+// working directory in the real file system due to a series of
+// unfortunate conditions interacting badly.
+// What's more, this path *must* be absolute on all (real)
+// filesystems, so just '/' won't work (e.g. on Win32).
+VFS->setCurrentWorkingDirectory("//./");
+  }
+
+  void TearDown() override {
+  }
+
+  void AddFile(const std::string &Filename, const std::string &Contents) {
+::time_t now;
+::time(&now);
+VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
+  }
+
+  void RemapFile(const std::string &Filename, const std::string &Contents) {
+RemappedFiles[Filename] = Contents;
+  }
+
+  std::unique_ptr ParseAST(const std::string &EntryFile) {
+PCHContainerOpts = std::make_shared();
+std::shared_ptr CI(new CompilerInvocation);
+CI->getFrontendOpts().Inputs.push_back(
+  FrontendInputFile(EntryFile, FrontendOptions::getInputKindForExtension(
+llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
+
+PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
+PPOpts.RemappedFilesKeepOriginalName = true;
+
+IntrusiveRefCntPtr
+  Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+
+FileManager *FileMgr = new FileManager(FSOpts, VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
+  CI, PCHContainerOpts, Diags, FileMgr, false, false,
+  /*PrecompilePreambleAfterNParses=*/1);
+return AST;
+  }
+
+  bool ReparseAST(const std::unique_ptr &AST) {
+bool reparseFailed = AST->Reparse(PCHContainerOpts, GetRemappedFiles(), VFS);
+return !reparseFailed;
+  }
+
+  unsigned GetFileReadCount(const std::string &Filename) const {
+return VFS->GetReadCount(Filename);
+  }
+
+private:
+  std::vector>
+  GetRemappedFiles() const {
+std::vector> Remapped;
+for (const auto &RemappedFile : RemappedFiles) {
+  std::unique_ptr buf = MemoryBuffer::getMemBufferCopy(
+RemappedFile.second, RemappedFile.first());
+  Remapped.emplace_back(RemappedFile.first(), buf.release());
+}
+return Remapped;
+  }
+};
+
+TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
+  std::string Header1 = "//./header1.h";
+  std::string Header2 = "//./header2.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(Header1, "");
+  AddFile(Header2, "#pragma once");
+  AddFile(MainName,
+"#include \"//./foo/../header1.h\"\n"
+"#include \"//./foo/../header2.h\"\

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

cameron314 added inline comments.



Comment at: include/clang/Frontend/PrecompiledPreamble.h:100
 
   /// PreambleBounds used to build the preamble
   PreambleBounds getBounds() const;

ilya-biryukov wrote:
> Not introduced by this change, but could you also add a full stop here for 
> constistency?
Sure.



Comment at: include/clang/Frontend/PrecompiledPreamble.h:103
 
+  /// The temporary file path at which the preamble PCH was placed
+  StringRef GetPCHPath() const { return PCHFile.getFilePath(); }

ilya-biryukov wrote:
> NIT: comment should end with a full stop.
OK!



Comment at: lib/Frontend/ASTUnit.cpp:1021
+  if (Buf)
+PCHFS->addFile(PCHFilename, 0, std::move(*Buf));
+  IntrusiveRefCntPtr

ilya-biryukov wrote:
> Maybe return original `VFS` if `PCHFilename` could not be read and not create 
> any empty overlays?
Makes sense, will do.



Comment at: lib/Frontend/ASTUnit.cpp:1053
+  IntrusiveRefCntPtr RealFS = vfs::getRealFileSystem();
+  if (OverrideMainBuffer && VFS && RealFS && VFS != RealFS &&
+  !VFS->exists(Preamble->GetPCHPath())) {

ilya-biryukov wrote:
> The check `&& RealFS` is redundant and can be removed.
It's not redundant, but looking at the implementation of 
`vfs::getRealFileSystem` it will always return a non-null pointer.


https://reviews.llvm.org/D37474



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

[PATCH] D37474: [PCH] Allow VFS to be used for tests that generate PCH files

[Cameron via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL312917: [PCH] Allow VFS to be used for tests that generate 
PCH files (authored by cameron314).

Changed prior to commit:
  https://reviews.llvm.org/D37474?vs=114604&id=114612#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37474

Files:
  cfe/trunk/include/clang/Frontend/PrecompiledPreamble.h
  cfe/trunk/lib/Frontend/ASTUnit.cpp
  cfe/trunk/unittests/Frontend/CMakeLists.txt
  cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp

Index: cfe/trunk/lib/Frontend/ASTUnit.cpp
===
--- cfe/trunk/lib/Frontend/ASTUnit.cpp
+++ cfe/trunk/lib/Frontend/ASTUnit.cpp
@@ -1009,6 +1009,24 @@
   }
 }
 
+static IntrusiveRefCntPtr createVFSOverlayForPreamblePCH(
+StringRef PCHFilename,
+IntrusiveRefCntPtr RealFS,
+IntrusiveRefCntPtr VFS) {
+  // We want only the PCH file from the real filesystem to be available,
+  // so we create an in-memory VFS with just that and overlay it on top.
+  auto Buf = RealFS->getBufferForFile(PCHFilename);
+  if (!Buf)
+return VFS;
+  IntrusiveRefCntPtr
+  PCHFS(new vfs::InMemoryFileSystem());
+  PCHFS->addFile(PCHFilename, 0, std::move(*Buf));
+  IntrusiveRefCntPtr
+  Overlay(new vfs::OverlayFileSystem(VFS));
+  Overlay->pushOverlay(PCHFS);
+  return Overlay;
+}
+
 /// Parse the source file into a translation unit using the given compiler
 /// invocation, replacing the current translation unit.
 ///
@@ -1030,6 +1048,24 @@
 Clang->setVirtualFileSystem(VFS);
   }
 
+  // Make sure we can access the PCH file even if we're using a VFS
+  if (!VFS && FileMgr)
+VFS = FileMgr->getVirtualFileSystem();
+  IntrusiveRefCntPtr RealFS = vfs::getRealFileSystem();
+  if (OverrideMainBuffer && VFS && RealFS && VFS != RealFS &&
+  !VFS->exists(Preamble->GetPCHPath())) {
+// We have a slight inconsistency here -- we're using the VFS to
+// read files, but the PCH was generated in the real file system.
+VFS = createVFSOverlayForPreamblePCH(Preamble->GetPCHPath(), RealFS, VFS);
+if (FileMgr) {
+  FileMgr = new FileManager(FileMgr->getFileSystemOpts(), VFS);
+  Clang->setFileManager(FileMgr.get());
+}
+else {
+  Clang->setVirtualFileSystem(VFS);
+}
+  }
+
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar
 CICleanup(Clang.get());
Index: cfe/trunk/unittests/Frontend/CMakeLists.txt
===
--- cfe/trunk/unittests/Frontend/CMakeLists.txt
+++ cfe/trunk/unittests/Frontend/CMakeLists.txt
@@ -6,6 +6,7 @@
   ASTUnitTest.cpp
   FrontendActionTest.cpp
   CodeGenActionTest.cpp
+  PCHPreambleTest.cpp
   )
 target_link_libraries(FrontendTests
   clangAST
Index: cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp
===
--- cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp
+++ cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp
@@ -0,0 +1,156 @@
+//-- unittests/Frontend/PCHPreambleTest.cpp - FrontendAction tests ---//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+  std::map ReadCounts;
+
+public:
+  ErrorOr> openFileForRead(const Twine &Path) override
+  {
+SmallVector PathVec;
+Path.toVector(PathVec);
+llvm::sys::path::remove_dots(PathVec, true);
+++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+return InMemoryFileSystem::openFileForRead(Path);
+  }
+
+  unsigned GetReadCount(const Twine &Path) const
+  {
+auto it = ReadCounts.find(Path.str());
+return it == ReadCounts.end() ? 0 : it->second;
+  }
+};
+
+class PCHPreambleTest : public ::testing::Test {
+  IntrusiveRefCntPtr VFS;
+  StringMap RemappedFiles;
+  std::shared_ptr PCHContainerOpts;
+  FileSystemOptions FSOpts;
+
+public:
+  void SetUp() override {
+VFS = new ReadCountingInMemoryFileSystem();
+// We need the working directory to be set to something absolute,
+// otherwise it ends up being inadvertently set to the current
+// working directory in the real file system due to a seri

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 115104.
cameron314 edited the summary of this revision.
cameron314 added a comment.

Alright, I've changed the patch so that the preamble takes into account the BOM 
presence and is invalidated when it changes. This automatically fixes all 
clients of `PrecompiledPreamble`, and ensures that all `SourceLocation`s are 
always consistent when using a PCH generated from the preamble.

I think this should do the trick!


https://reviews.llvm.org/D37491

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  include/clang/Lex/Lexer.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp
  lib/Lex/Lexer.cpp
  lib/Lex/Preprocessor.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -153,4 +153,48 @@
   ASSERT_EQ(initialCounts[2], GetFileReadCount(Header2));
 }
 
+TEST_F(PCHPreambleTest, ParseWithBom) {
+  std::string Header = "//./header.h";
+  std::string Main = "//./main.cpp";
+  AddFile(Header, "int random() { return 4; }");
+  AddFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() -2; }");
+
+  std::unique_ptr AST(ParseAST(Main));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  unsigned HeaderReadCount = GetFileReadCount(Header);
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  
+  // Check preamble PCH was really reused
+  ASSERT_EQ(HeaderReadCount, GetFileReadCount(Header));
+
+  // Remove BOM
+  RemapFile(Main,
+"#include \"//./header.h\"\n"
+"int main() { return random() -2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_LE(HeaderReadCount, GetFileReadCount(Header));
+  HeaderReadCount = GetFileReadCount(Header);
+
+  // Add BOM back
+  RemapFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() -2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_LE(HeaderReadCount, GetFileReadCount(Header));
+}
+
 } // anonymous namespace
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -516,9 +516,9 @@
 // If we've been asked to skip bytes in the main file (e.g., as part of a
 // precompiled preamble), do so now.
 if (SkipMainFilePreamble.first > 0)
-  CurLexer->SkipBytes(SkipMainFilePreamble.first, 
-  SkipMainFilePreamble.second);
-
+  CurLexer->SetByteOffset(SkipMainFilePreamble.first,
+  SkipMainFilePreamble.second);
+
 // Tell the header info that the main file was entered.  If the file is later
 // #imported, it won't be re-entered.
 if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID))
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -552,9 +552,9 @@
 
 } // end anonymous namespace
 
-std::pair Lexer::ComputePreamble(StringRef Buffer,
- const LangOptions &LangOpts,
- unsigned MaxLines) {
+PreambleBounds Lexer::ComputePreamble(StringRef Buffer,
+  const LangOptions &LangOpts,
+  unsigned MaxLines) {
   // Create a lexer starting at the beginning of the file. Note that we use a
   // "fake" file source location at offset 1 so that the lexer will track our
   // position within the file.
@@ -688,7 +688,8 @@
   else
 End = TheTok.getLocation();
 
-  return std::make_pair(End.getRawEncoding() - StartLoc.getRawEncoding(),
+  return PreambleBounds(StartLoc.getRawEncoding() - FileLoc.getRawEncoding(),
+End.getRawEncoding() - StartLoc.getRawEncoding(),
 TheTok.isAtStartOfLine());
 }
 
@@ -1394,9 +1395,9 @@
 // Helper methods for lexing.
 //===--===//
 
-/// \brief Routine that indiscriminately skips bytes in the source file.
-void Lexer::SkipBytes(unsigned Bytes, bool StartOfLine) {
-  BufferPtr += Bytes;
+/// \brief Routine that indiscriminately sets the offset into the source file.
+void Lexer::SetByteOffset(unsigned Offset, bool StartOfLine) {
+  BufferPtr = BufferStart + Offset;
   if (BufferPtr > BufferEnd)
 BufferPtr = BufferEnd;
   // FIXME: What exactly does the StartOfLine bit mean?  There are two
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Fron

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 added inline comments.



Comment at: include/clang/Lex/Lexer.h:52
+  /// a BOM is present at the start of the file.
+  unsigned StartOffset;
+  /// \brief Size of the preamble in bytes.

ilya-biryukov wrote:
> We could simplify it further by removing `StartOffset`, leaving only `Size`.
> If you look at the code, it always uses `StartOffset + Size` now, which is 
> effectively size with BOM.
> What do you think?
Yeah, I thought of that, but it's still nice to have the two separated.

ASTUnit.cpp, for example, checks if `Size != 0` to determine if there's a 
preamble (without considering the offset). This isn't in the diff because it 
was already like that.


https://reviews.llvm.org/D37491



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

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 added inline comments.



Comment at: include/clang/Lex/Lexer.h:52
+  /// a BOM is present at the start of the file.
+  unsigned StartOffset;
+  /// \brief Size of the preamble in bytes.

ilya-biryukov wrote:
> cameron314 wrote:
> > ilya-biryukov wrote:
> > > We could simplify it further by removing `StartOffset`, leaving only 
> > > `Size`.
> > > If you look at the code, it always uses `StartOffset + Size` now, which 
> > > is effectively size with BOM.
> > > What do you think?
> > Yeah, I thought of that, but it's still nice to have the two separated.
> > 
> > ASTUnit.cpp, for example, checks if `Size != 0` to determine if there's a 
> > preamble (without considering the offset). This isn't in the diff because 
> > it was already like that.
> Could we simply return `Size = 0` from `ComputePreambleBounds` if we simply 
> skipped BOM and the preamble itself is empty?
> My concern is that currently it's very easy to forget adding `StartOffset` 
> and simply use `Size` when writing code that uses `PreambleBounds`. If we 
> only have `Size`, probability of mistakes is much lower.
Alright, sold. There's already other places that use the size without checking 
the offset, it turns out.


https://reviews.llvm.org/D37491



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

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 116043.
cameron314 added a comment.

Final diff. Test passes!


https://reviews.llvm.org/D37491

Files:
  include/clang/Frontend/PrecompiledPreamble.h
  include/clang/Lex/Lexer.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp
  lib/Lex/Lexer.cpp
  lib/Lex/Preprocessor.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -153,4 +153,48 @@
   ASSERT_EQ(initialCounts[2], GetFileReadCount(Header2));
 }
 
+TEST_F(PCHPreambleTest, ParseWithBom) {
+  std::string Header = "//./header.h";
+  std::string Main = "//./main.cpp";
+  AddFile(Header, "int random() { return 4; }");
+  AddFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() -2; }");
+
+  std::unique_ptr AST(ParseAST(Main));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  unsigned HeaderReadCount = GetFileReadCount(Header);
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  
+  // Check preamble PCH was really reused
+  ASSERT_EQ(HeaderReadCount, GetFileReadCount(Header));
+
+  // Remove BOM
+  RemapFile(Main,
+"#include \"//./header.h\"\n"
+"int main() { return random() -2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_LE(HeaderReadCount, GetFileReadCount(Header));
+  HeaderReadCount = GetFileReadCount(Header);
+
+  // Add BOM back
+  RemapFile(Main,
+"\xef\xbb\xbf"
+"#include \"//./header.h\"\n"
+"int main() { return random() -2; }");
+
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  ASSERT_LE(HeaderReadCount, GetFileReadCount(Header));
+}
+
 } // anonymous namespace
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -516,9 +516,9 @@
 // If we've been asked to skip bytes in the main file (e.g., as part of a
 // precompiled preamble), do so now.
 if (SkipMainFilePreamble.first > 0)
-  CurLexer->SkipBytes(SkipMainFilePreamble.first, 
-  SkipMainFilePreamble.second);
-
+  CurLexer->SetByteOffset(SkipMainFilePreamble.first,
+  SkipMainFilePreamble.second);
+
 // Tell the header info that the main file was entered.  If the file is later
 // #imported, it won't be re-entered.
 if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID))
Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -552,9 +552,9 @@
 
 } // end anonymous namespace
 
-std::pair Lexer::ComputePreamble(StringRef Buffer,
- const LangOptions &LangOpts,
- unsigned MaxLines) {
+PreambleBounds Lexer::ComputePreamble(StringRef Buffer,
+  const LangOptions &LangOpts,
+  unsigned MaxLines) {
   // Create a lexer starting at the beginning of the file. Note that we use a
   // "fake" file source location at offset 1 so that the lexer will track our
   // position within the file.
@@ -688,7 +688,7 @@
   else
 End = TheTok.getLocation();
 
-  return std::make_pair(End.getRawEncoding() - StartLoc.getRawEncoding(),
+  return PreambleBounds(End.getRawEncoding() - FileLoc.getRawEncoding(),
 TheTok.isAtStartOfLine());
 }
 
@@ -1394,9 +1394,9 @@
 // Helper methods for lexing.
 //===--===//
 
-/// \brief Routine that indiscriminately skips bytes in the source file.
-void Lexer::SkipBytes(unsigned Bytes, bool StartOfLine) {
-  BufferPtr += Bytes;
+/// \brief Routine that indiscriminately sets the offset into the source file.
+void Lexer::SetByteOffset(unsigned Offset, bool StartOfLine) {
+  BufferPtr = BufferStart + Offset;
   if (BufferPtr > BufferEnd)
 BufferPtr = BufferEnd;
   // FIXME: What exactly does the StartOfLine bit mean?  There are two
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -195,8 +195,7 @@
 PreambleBounds clang::ComputePreambleBounds(const LangOptions &LangOpts,
 llvm::MemoryBuffer *Buffer,
 unsigned MaxLines) {
-  auto Pre = Lexer::ComputePreamble(Buffer->getBuffer(), LangOpts, MaxLines);
-  return PreambleBounds(Pre.first, Pre.second);
+  re

[PATCH] D37491: [Preamble] Fixed preamble breaking with BOM presence (and particularly, fluctuating BOM presence)

[Cameron via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL313796: [PCH] Fixed preamble breaking with BOM presence (and 
particularly, fluctuating… (authored by cameron314).

Changed prior to commit:
  https://reviews.llvm.org/D37491?vs=116043&id=116049#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37491

Files:
  cfe/trunk/include/clang/Frontend/PrecompiledPreamble.h
  cfe/trunk/include/clang/Lex/Lexer.h
  cfe/trunk/include/clang/Lex/PreprocessorOptions.h
  cfe/trunk/lib/Frontend/FrontendActions.cpp
  cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
  cfe/trunk/lib/Lex/Lexer.cpp
  cfe/trunk/lib/Lex/Preprocessor.cpp
  cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp

Index: cfe/trunk/lib/Lex/Preprocessor.cpp
===
--- cfe/trunk/lib/Lex/Preprocessor.cpp
+++ cfe/trunk/lib/Lex/Preprocessor.cpp
@@ -516,9 +516,9 @@
 // If we've been asked to skip bytes in the main file (e.g., as part of a
 // precompiled preamble), do so now.
 if (SkipMainFilePreamble.first > 0)
-  CurLexer->SkipBytes(SkipMainFilePreamble.first, 
-  SkipMainFilePreamble.second);
-
+  CurLexer->SetByteOffset(SkipMainFilePreamble.first,
+  SkipMainFilePreamble.second);
+
 // Tell the header info that the main file was entered.  If the file is later
 // #imported, it won't be re-entered.
 if (const FileEntry *FE = SourceMgr.getFileEntryForID(MainFileID))
Index: cfe/trunk/lib/Lex/Lexer.cpp
===
--- cfe/trunk/lib/Lex/Lexer.cpp
+++ cfe/trunk/lib/Lex/Lexer.cpp
@@ -552,9 +552,9 @@
 
 } // end anonymous namespace
 
-std::pair Lexer::ComputePreamble(StringRef Buffer,
- const LangOptions &LangOpts,
- unsigned MaxLines) {
+PreambleBounds Lexer::ComputePreamble(StringRef Buffer,
+  const LangOptions &LangOpts,
+  unsigned MaxLines) {
   // Create a lexer starting at the beginning of the file. Note that we use a
   // "fake" file source location at offset 1 so that the lexer will track our
   // position within the file.
@@ -688,7 +688,7 @@
   else
 End = TheTok.getLocation();
 
-  return std::make_pair(End.getRawEncoding() - StartLoc.getRawEncoding(),
+  return PreambleBounds(End.getRawEncoding() - FileLoc.getRawEncoding(),
 TheTok.isAtStartOfLine());
 }
 
@@ -1394,9 +1394,9 @@
 // Helper methods for lexing.
 //===--===//
 
-/// \brief Routine that indiscriminately skips bytes in the source file.
-void Lexer::SkipBytes(unsigned Bytes, bool StartOfLine) {
-  BufferPtr += Bytes;
+/// \brief Routine that indiscriminately sets the offset into the source file.
+void Lexer::SetByteOffset(unsigned Offset, bool StartOfLine) {
+  BufferPtr = BufferStart + Offset;
   if (BufferPtr > BufferEnd)
 BufferPtr = BufferEnd;
   // FIXME: What exactly does the StartOfLine bit mean?  There are two
Index: cfe/trunk/lib/Frontend/FrontendActions.cpp
===
--- cfe/trunk/lib/Frontend/FrontendActions.cpp
+++ cfe/trunk/lib/Frontend/FrontendActions.cpp
@@ -591,7 +591,7 @@
   auto Buffer = CI.getFileManager().getBufferForFile(getCurrentFile());
   if (Buffer) {
 unsigned Preamble =
-Lexer::ComputePreamble((*Buffer)->getBuffer(), CI.getLangOpts()).first;
+Lexer::ComputePreamble((*Buffer)->getBuffer(), CI.getLangOpts()).Size;
 llvm::outs().write((*Buffer)->getBufferStart(), Preamble);
   }
 }
Index: cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
===
--- cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
+++ cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
@@ -195,8 +195,7 @@
 PreambleBounds clang::ComputePreambleBounds(const LangOptions &LangOpts,
 llvm::MemoryBuffer *Buffer,
 unsigned MaxLines) {
-  auto Pre = Lexer::ComputePreamble(Buffer->getBuffer(), LangOpts, MaxLines);
-  return PreambleBounds(Pre.first, Pre.second);
+  return Lexer::ComputePreamble(Buffer->getBuffer(), LangOpts, MaxLines);
 }
 
 llvm::ErrorOr PrecompiledPreamble::Build(
Index: cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp
===
--- cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp
+++ cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp
@@ -153,4 +153,48 @@
   ASSERT_EQ(initialCounts[2], GetFileReadCount(Header2));
 }
 
+TEST_F(PCHPreambleTest, ParseWithBom) {
+  std::string Header = "//./header.h";
+  std::string Main = "//./main.cpp";
+  AddFile(Header, "int random() { return 4; }");
+  AddFile(Main,
+"\xef\xbb

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Brilliant, didn't know `isInPreambleFileID` existed. All my tests pass for me 
now with that change, thanks :-)
I'll update the patch on Monday.


https://reviews.llvm.org/D20124



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 125333.
cameron314 added a comment.

Here's the final patch that fixes `clang_getSkippedRegions` with regions in the 
preamble (as well as serializing the skipped regions in the PCH file).


https://reviews.llvm.org/D20124

Files:
  include/clang/Lex/PreprocessingRecord.h
  include/clang/Serialization/ASTBitCodes.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/Module.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PreprocessingRecord.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  tools/libclang/CIndex.cpp
  unittests/libclang/LibclangTest.cpp

Index: unittests/libclang/LibclangTest.cpp
===
--- unittests/libclang/LibclangTest.cpp
+++ unittests/libclang/LibclangTest.cpp
@@ -572,3 +572,81 @@
   EXPECT_EQ(0U, clang_getNumDiagnostics(ClangTU));
   DisplayDiagnostics();
 }
+
+TEST_F(LibclangReparseTest, PreprocessorSkippedRanges) {
+  std::string Header = "header.h", Main = "main.cpp";
+  WriteFile(Header,
+"#ifdef MANGOS\n"
+"printf(\"mmm\");\n"
+"#endif");
+  WriteFile(Main,
+"#include \"header.h\"\n"
+"#ifdef GUAVA\n"
+"#endif\n"
+"#ifdef KIWIS\n"
+"printf(\"mmm!!\");\n"
+"#endif");
+
+  for (int i = 0; i != 3; ++i) {
+unsigned flags = TUFlags | CXTranslationUnit_PrecompiledPreamble;
+if (i == 2)
+  flags |= CXTranslationUnit_CreatePreambleOnFirstParse;
+
+// parse once
+ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0,
+ nullptr, 0, flags);
+if (i != 0) {
+  // reparse
+  ASSERT_TRUE(ReparseTU(0, nullptr /* No unsaved files. */));
+}
+
+// Check all ranges are there
+CXSourceRangeList *Ranges = clang_getAllSkippedRanges(ClangTU);
+EXPECT_EQ(3U, Ranges->count);
+
+CXSourceLocation cxl;
+unsigned line;
+cxl = clang_getRangeStart(Ranges->ranges[0]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(1U, line);
+cxl = clang_getRangeEnd(Ranges->ranges[0]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(3U, line);
+
+cxl = clang_getRangeStart(Ranges->ranges[1]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(2U, line);
+cxl = clang_getRangeEnd(Ranges->ranges[1]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(3U, line);
+
+cxl = clang_getRangeStart(Ranges->ranges[2]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(4U, line);
+cxl = clang_getRangeEnd(Ranges->ranges[2]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(6U, line);
+
+clang_disposeSourceRangeList(Ranges);
+
+// Check obtaining ranges by each file works
+CXFile cxf = clang_getFile(ClangTU, Header.c_str());
+Ranges = clang_getSkippedRanges(ClangTU, cxf);
+EXPECT_EQ(1U, Ranges->count);
+cxl = clang_getRangeStart(Ranges->ranges[0]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(1U, line);
+clang_disposeSourceRangeList(Ranges);
+
+cxf = clang_getFile(ClangTU, Main.c_str());
+Ranges = clang_getSkippedRanges(ClangTU, cxf);
+EXPECT_EQ(2U, Ranges->count);
+cxl = clang_getRangeStart(Ranges->ranges[0]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(2U, line);
+cxl = clang_getRangeStart(Ranges->ranges[1]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(4U, line);
+clang_disposeSourceRangeList(Ranges);
+  }
+}
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8079,13 +8079,16 @@
   SourceManager &sm = Ctx.getSourceManager();
   FileEntry *fileEntry = static_cast(file);
   FileID wantedFileID = sm.translateFile(fileEntry);
+  bool isMainFile = wantedFileID == sm.getMainFileID();
 
   const std::vector &SkippedRanges = ppRec->getSkippedRanges();
   std::vector wantedRanges;
   for (std::vector::const_iterator i = SkippedRanges.begin(), ei = SkippedRanges.end();
i != ei; ++i) {
 if (sm.getFileID(i->getBegin()) == wantedFileID || sm.getFileID(i->getEnd()) == wantedFileID)
   wantedRanges.push_back(*i);
+else if (isMainFile && (astUnit->isInPreambleFileID(i->getBegin()) || astUnit->isInPreambleFileID(i->getEnd(
+  wantedRanges.push_back(*i);
   }
 
   skipped->count = wantedRanges.size();
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1104,6 +1104,7 @@
   RECORD(UNUSED_FILESCOPED_DECLS);
   RECORD(PPD_ENTITIES_OFFSETS);
   RECORD(VTABLE_USES);
+  RECORD(PPD_SKIPPED_RANGES);
   RECORD(R

[PATCH] D39903: [libclang] Allow pretty printing declarations

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Locally we've done something similar (adding a 
`clang_getCursorPrettyPrintedDeclaration` function, though without exposing the 
`PrintingPolicy`) and overhauled `DeclPrinter` to produce proper pretty names. 
This is a hack that works well for us, but can never be upstreamed since it 
breaks too much existing code (and some of the printing decisions are 
subjective). Your way is better.

I can point out differences in our implementations if you like. The diff is 
rather long, though.


Repository:
  rC Clang

https://reviews.llvm.org/D39903



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

[PATCH] D36390: Fix overloaded static functions in SemaCodeComplete

[Cameron via Phabricator via cfe-commits]

cameron314 added inline comments.



Comment at: cfe/trunk/lib/Sema/SemaOverload.cpp:6365
cast(FD)->getParent(), ObjectType,
-   ObjectClassification, Args.slice(1), CandidateSet,
+   ObjectClassification, FunctionArgs, CandidateSet,
SuppressUserConversions, PartialOverloading);

yvvan wrote:
> cameron314 wrote:
> > This breaks normal (non-static) method overload resolution, since the 
> > `this` argument is now passed to `AddMethodCandidate` which is not 
> > expecting it. It always thinks too many arguments are passed to methods 
> > with no parameters; most other calls to `AddMethodCandidate` in 
> > SemaOverload.cpp don't pass the implicit `this`.
> This code is correct. It is sliced at line 6361 - only in the case when the 
> args size is greater than 0.
Hmm, you're right, I didn't see that. That line was missing after a rebase on 
our side, my fault for not properly cross-referencing the diffs. Sorry for the 
false alarm.

But shouldn't the slice only be done now when `Args.size() > 0 && (!Args[0] || 
(FirstArgumentIsBase && isa(FD) && 
!isa(FD)))`?



Comment at: cfe/trunk/lib/Sema/SemaOverload.cpp:6396
   } else {
 AddTemplateOverloadCandidate(FunTmpl, F.getPair(),
  ExplicitTemplateArgs, Args,

yvvan wrote:
> cameron314 wrote:
> > The same slice that was added above needs to be done here for templated 
> > static methods that are accessed via a non-static object.
> It might be the case. You can add that :)
> I did not check template cases in details.
It is the case, I have parameter completion tests that fail without this :-)
I'll commit in a few days.


Repository:
  rL LLVM

https://reviews.llvm.org/D36390



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

It's been a while since I was in this code, but I seem to recall the file 
needed to exist on disk in order to uniquely identify it (via inode). Does this 
break the up-to-date check?


Repository:
  rC Clang

https://reviews.llvm.org/D41005



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

[PATCH] D19031: [clang-format] Flexible line endings

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

@MyDeveloperDay: @mxbOctasic has moved on but I'm still maintaining this patch 
upstream, so I'm still interested in having it merged if possible.

The advantage of having two options is that a default line ending can still be 
specified when none can be derived (this is important to our use case where we 
sometimes pass individual lines to clang-format).


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

https://reviews.llvm.org/D19031



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

[PATCH] D19031: [clang-format] Flexible line endings

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 229533.
cameron314 added a comment.

Rebased diff on latest upstream.


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

https://reviews.llvm.org/D19031

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12500,6 +12500,7 @@
   CHECK_PARSE_BOOL(BreakStringLiterals);
   CHECK_PARSE_BOOL(CompactNamespaces);
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
+  CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
@@ -12528,6 +12529,7 @@
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
+  CHECK_PARSE_BOOL(UseCRLF);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
@@ -14056,6 +14058,94 @@
 format("/*\r\n"
"\r\r\r\n"
"*/"));
+
+  FormatStyle style = getLLVMStyle();
+
+  style.DeriveLineEnding = true;
+  style.UseCRLF = false;
+  EXPECT_EQ("union FooBarBazQux {\n"
+"  int foo;\n"
+"  int bar;\n"
+"  int baz;\n"
+"};",
+format("union FooBarBazQux {\r\n"
+   "  int foo;\n"
+   "  int bar;\r\n"
+   "  int baz;\n"
+   "};",
+   style));
+  style.UseCRLF = true;
+  EXPECT_EQ("union FooBarBazQux {\r\n"
+"  int foo;\r\n"
+"  int bar;\r\n"
+"  int baz;\r\n"
+"};",
+format("union FooBarBazQux {\r\n"
+   "  int foo;\n"
+   "  int bar;\r\n"
+   "  int baz;\n"
+   "};",
+   style));
+
+  style.DeriveLineEnding = false;
+  style.UseCRLF = false;
+  EXPECT_EQ("union FooBarBazQux {\n"
+"  int foo;\n"
+"  int bar;\n"
+"  int baz;\n"
+"  int qux;\n"
+"};",
+format("union FooBarBazQux {\r\n"
+   "  int foo;\n"
+   "  int bar;\r\n"
+   "  int baz;\n"
+   "  int qux;\r\n"
+   "};",
+   style));
+  style.UseCRLF = true;
+  EXPECT_EQ("union FooBarBazQux {\r\n"
+"  int foo;\r\n"
+"  int bar;\r\n"
+"  int baz;\r\n"
+"  int qux;\r\n"
+"};",
+format("union FooBarBazQux {\r\n"
+   "  int foo;\n"
+   "  int bar;\r\n"
+   "  int baz;\n"
+   "  int qux;\n"
+   "};",
+   style));
+
+  style.DeriveLineEnding = true;
+  style.UseCRLF = false;
+  EXPECT_EQ("union FooBarBazQux {\r\n"
+"  int foo;\r\n"
+"  int bar;\r\n"
+"  int baz;\r\n"
+"  int qux;\r\n"
+"};",
+format("union FooBarBazQux {\r\n"
+   "  int foo;\n"
+   "  int bar;\r\n"
+   "  int baz;\n"
+   "  int qux;\r\n"
+   "};",
+   style));
+  style.UseCRLF = true;
+  EXPECT_EQ("union FooBarBazQux {\n"
+"  int foo;\n"
+"  int bar;\n"
+"  int baz;\n"
+"  int qux;\n"
+"};",
+format("union FooBarBazQux {\r\n"
+   "  int foo;\n"
+   "  int bar;\r\n"
+   "  int baz;\n"
+   "  int qux;\n"
+   "};",
+   style));
 }
 
 TEST_F(FormatTest, MunchSemicolonAfterBlocks) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -467,6 +467,7 @@
Style.ConstructorInitializerIndentWidth);
 IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
 IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
+IO.mapOptional("DeriveLineEnding", Style.DeriveLineEnding);
 IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
 IO.mapOptional("DisableFormat", Style.DisableFormat);
 IO.mapOptional("ExperimentalAutoDetectBinPacking",
@@ -546,6 +547,7 @@
 IO.mapOptional("StatementMacros", Style.StatementMacros);
 IO.mapOptional("TabWidth", Style.TabWidth);
 IO.mapOptional("TypenameMacros", Style.TypenameMacros);
+IO.mapOptional("UseCRLF", Style.UseCRLF);
 IO.mapOptiona

[PATCH] D19031: [clang-format] Flexible line endings

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

I've got a rebased diff but Phabricator won't let me attach it to this 
differential review (too old?). What do I do?

EDIT: Ah, it must be because I'm not the owner, despite it being editable by 
all.


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

https://reviews.llvm.org/D19031



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

I'll rebase the patch and add a test. Thanks for looking at this!


Repository:
  rL LLVM

https://reviews.llvm.org/D20124



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

[PATCH] D36390: Fix overloaded static functions in SemaCodeComplete

[Cameron via Phabricator via cfe-commits]

cameron314 added inline comments.



Comment at: cfe/trunk/lib/Sema/SemaOverload.cpp:6365
cast(FD)->getParent(), ObjectType,
-   ObjectClassification, Args.slice(1), CandidateSet,
+   ObjectClassification, FunctionArgs, CandidateSet,
SuppressUserConversions, PartialOverloading);

This breaks normal (non-static) method overload resolution, since the `this` 
argument is now passed to `AddMethodCandidate` which is not expecting it. It 
always thinks too many arguments are passed to methods with no parameters; most 
other calls to `AddMethodCandidate` in SemaOverload.cpp don't pass the implicit 
`this`.


Repository:
  rL LLVM

https://reviews.llvm.org/D36390



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

[PATCH] D36390: Fix overloaded static functions in SemaCodeComplete

[Cameron via Phabricator via cfe-commits]

cameron314 added inline comments.



Comment at: cfe/trunk/lib/Sema/SemaOverload.cpp:6396
   } else {
 AddTemplateOverloadCandidate(FunTmpl, F.getPair(),
  ExplicitTemplateArgs, Args,

The same slice that was added above needs to be done here for templated static 
methods that are accessed via a non-static object.


Repository:
  rL LLVM

https://reviews.llvm.org/D36390



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 updated this revision to Diff 123044.
cameron314 added a comment.

Fully rebased, with a test.


https://reviews.llvm.org/D20124

Files:
  include/clang/Lex/PreprocessingRecord.h
  include/clang/Serialization/ASTBitCodes.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/Module.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PreprocessingRecord.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  unittests/libclang/LibclangTest.cpp

Index: unittests/libclang/LibclangTest.cpp
===
--- unittests/libclang/LibclangTest.cpp
+++ unittests/libclang/LibclangTest.cpp
@@ -572,3 +572,52 @@
   EXPECT_EQ(0U, clang_getNumDiagnostics(ClangTU));
   DisplayDiagnostics();
 }
+
+TEST_F(LibclangReparseTest, PreprocessorSkippedRanges) {
+  std::string Header = "header.h", Main = "main.cpp";
+  WriteFile(Header,
+"#ifdef MANGOS\n"
+"printf(\"mmm\");\n"
+"#endif");
+  WriteFile(Main,
+"#include \"header.h\"\n"
+"#ifdef KIWIS\n"
+"printf(\"mmm!!\");\n"
+"#endif");
+
+  unsigned flags = TUFlags |
+CXTranslationUnit_PrecompiledPreamble;//|
+  //CXTranslationUnit_CreatePreambleOnFirstParse;
+  for (int i = 0; i != 5; ++i) {
+if (i == 0) {
+  // first parse
+  ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0,
+   nullptr, 0, flags);
+}
+else {
+  // reparse
+  ASSERT_TRUE(ReparseTU(0, nullptr /* No unsaved files. */));
+}
+
+CXSourceRangeList *Ranges = clang_getAllSkippedRanges(ClangTU);
+EXPECT_EQ(2U, Ranges->count);
+
+CXSourceLocation cxl;
+unsigned line;
+cxl = clang_getRangeStart(Ranges->ranges[0]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(1U, line);
+cxl = clang_getRangeEnd(Ranges->ranges[0]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(3U, line);
+
+cxl = clang_getRangeStart(Ranges->ranges[1]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(2U, line);
+cxl = clang_getRangeEnd(Ranges->ranges[1]);
+clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
+EXPECT_EQ(4U, line);
+
+clang_disposeSourceRangeList(Ranges);
+  }
+}
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1104,6 +1104,7 @@
   RECORD(UNUSED_FILESCOPED_DECLS);
   RECORD(PPD_ENTITIES_OFFSETS);
   RECORD(VTABLE_USES);
+  RECORD(PPD_SKIPPED_RANGES);
   RECORD(REFERENCED_SELECTOR_POOL);
   RECORD(TU_UPDATE_LEXICAL);
   RECORD(SEMA_DECL_REFS);
@@ -2725,6 +2726,26 @@
 Stream.EmitRecordWithBlob(PPEOffsetAbbrev, Record,
   bytes(PreprocessedEntityOffsets));
   }
+
+  // Write the skipped region table for the preprocessing record.
+  ArrayRef SkippedRanges = PPRec.getSkippedRanges();
+  if (SkippedRanges.size() > 0) {
+std::vector SerializedSkippedRanges;
+SerializedSkippedRanges.reserve(SkippedRanges.size());
+for (auto const& Range : SkippedRanges)
+  SerializedSkippedRanges.emplace_back(Range);
+
+using namespace llvm;
+auto Abbrev = std::make_shared();
+Abbrev->Add(BitCodeAbbrevOp(PPD_SKIPPED_RANGES));
+Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
+unsigned PPESkippedRangeAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
+
+Record.clear();
+Record.push_back(PPD_SKIPPED_RANGES);
+Stream.EmitRecordWithBlob(PPESkippedRangeAbbrev, Record,
+  bytes(SerializedSkippedRanges));
+  }
 }
 
 unsigned ASTWriter::getLocalOrImportedSubmoduleID(Module *Mod) {
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -3214,6 +3214,24 @@
   break;
 }
 
+case PPD_SKIPPED_RANGES: {
+  F.PreprocessedSkippedRangeOffsets = (const PPSkippedRange*)Blob.data();
+  assert(Blob.size() % sizeof(PPSkippedRange) == 0);
+  F.NumPreprocessedSkippedRanges = Blob.size() / sizeof(PPSkippedRange);
+
+  if (!PP.getPreprocessingRecord())
+PP.createPreprocessingRecord();
+  if (!PP.getPreprocessingRecord()->getExternalSource())
+PP.getPreprocessingRecord()->SetExternalSource(*this);
+  F.BasePreprocessedSkippedRangeID = PP.getPreprocessingRecord()
+  ->allocateSkippedRanges(F.NumPreprocessedSkippedRanges);
+  
+  if (F.NumPreprocessedSkippedRanges > 0)
+GlobalSkippedRangeMap.insert(
+std::make_pair(F.BasePreprocessedSkippedRangeID, &F));
+  break;
+}
+
 case DECL_UPDATE_OFFSETS:
   if (Record.size() % 2 != 0) {
 Error("invalid DECL_UPDATE_OFFSETS block in AST file");
@@ -5379,6 +5397,20 @@
  Mod.FileSo

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 marked an inline comment as done.
cameron314 added a comment.

- Well that's odd, because the test definitely fails for me without the patch. 
I'm only a few days behind the trunk.
- I'm looking at your test case now. I can reproduce it even with the patch; 
I'm investigating what's happening.
- I've fixed the warning, thanks! Using MSVC on my end which doesn't emit that 
particular warning.


https://reviews.llvm.org/D20124



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Alright, with my patch the `c-index-test` *does* correctly serialize and 
restore the skipped ranges; the problem is that it searches for only ranges 
matching the target file. When there's a preamble, it's seen as a different 
file than the main file, even though they're really one and the same in this 
case.

So my test, which uses `clang_getAllSkippedRanges`, passes, but `c-index-test`, 
which uses `clang_getSkippedRanges`, fails. I'll update my test to use both.

I'm trying to figure out how to fix this.


https://reviews.llvm.org/D20124



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Well, it seems like preamble PCH source location translation is fundamentally 
broken. The entry file has a single, positive file ID. The preamble PCH is 
treated as an imported module, so it has a negative file ID for the part that 
overlaps the preamble of the entry file. That means locations in the preamble 
part of the entry file can have two different file IDs depending on how they 
are arrived at.

I really don't know how to fix this. Any ideas?


https://reviews.llvm.org/D20124



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

In https://reviews.llvm.org/D20124#928552, @ilya-biryukov wrote:

> Why do we store raw source locations in `PPSkippedRange`? Would storing 
> `SourceLocation` and using  `ASTWriter::AddSourceLocation` and `ASTReader:: 
> ReadSourceLocation` do the trick?


I followed the pattern used to store `PPEntityOffset`s; it allows the entire 
array to be written in one chunk. Using 
`AddSourceLocation`/`ReadSourceLocation` boils down to the same thing -- 
`AddSourceLocation` simply rotates by one bit, and `ReadSourceLocation` simply 
rotates back and calls `ASTReader::TranslateSourceLocation` (which I call from 
`AST::ReadSkippedRange` explicitly instead). So the source location translation 
path is exactly the same either way.

The thing is, the PCH created for the preamble is imported as a module, meaning 
the part of the source code that overlaps the preamble of the entry file has a 
different file ID from the entry file itself. Is there any way to force source 
locations in the preamble to map to the actual entry file instead of the 
module's version of the entry file?


https://reviews.llvm.org/D20124



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

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

>> Thanks, I'll check these out! Btw, I noticed that the clang-format tests are 
>> non-Windows due to path assumptions. Is this a lost cause, or just something 
>> no one's bothered to look into yet?
> 
> No one's bothered looking into it yet.

Which tests? We run the unit tests (FormatTests.exe) locally just fine on 
Windows.


https://reviews.llvm.org/D27440



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

Excellent, I'll rebase and commit. Thanks everyone for your patience!


https://reviews.llvm.org/D20124



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

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

[Cameron via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC322503: [PCH] Serialize skipped preprocessor ranges 
(authored by cameron314, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D20124?vs=125333&id=129892#toc

Repository:
  rC Clang

https://reviews.llvm.org/D20124

Files:
  include/clang/Lex/PreprocessingRecord.h
  include/clang/Serialization/ASTBitCodes.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/Module.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PreprocessingRecord.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  tools/libclang/CIndex.cpp
  unittests/libclang/LibclangTest.cpp

Index: include/clang/Lex/PreprocessingRecord.h
===
--- include/clang/Lex/PreprocessingRecord.h
+++ include/clang/Lex/PreprocessingRecord.h
@@ -297,6 +297,9 @@
 FileID FID) {
   return None;
 }
+
+/// \brief Read a preallocated skipped range from the external source.
+virtual SourceRange ReadSkippedRange(unsigned Index) = 0;
   };
   
   /// \brief A record of the steps taken while preprocessing a source file,
@@ -322,6 +325,8 @@
 /// \brief The set of ranges that were skipped by the preprocessor,
 std::vector SkippedRanges;
 
+bool SkippedRangesAllLoaded = true;
+
 /// \brief Global (loaded or local) ID for a preprocessed entity.
 /// Negative values are used to indicate preprocessed entities
 /// loaded from the external source while non-negative values are used to
@@ -377,6 +382,16 @@
 /// corresponds to the first newly-allocated entity.
 unsigned allocateLoadedEntities(unsigned NumEntities);
 
+/// \brief Allocate space for a new set of loaded preprocessed skipped
+/// ranges.
+///
+/// \returns The index into the set of loaded preprocessed ranges, which
+/// corresponds to the first newly-allocated range.
+unsigned allocateSkippedRanges(unsigned NumRanges);
+
+/// \brief Ensures that all external skipped ranges have been loaded.
+void ensureSkippedRangesLoaded();
+
 /// \brief Register a new macro definition.
 void RegisterMacroDefinition(MacroInfo *Macro, MacroDefinitionRecord *Def);
 
@@ -499,7 +514,8 @@
 MacroDefinitionRecord *findMacroDefinition(const MacroInfo *MI);
 
 /// \brief Retrieve all ranges that got skipped while preprocessing.
-const std::vector &getSkippedRanges() const {
+const std::vector &getSkippedRanges() {
+  ensureSkippedRangesLoaded();
   return SkippedRanges;
 }
 
Index: include/clang/Serialization/Module.h
===
--- include/clang/Serialization/Module.h
+++ include/clang/Serialization/Module.h
@@ -324,6 +324,12 @@
   const PPEntityOffset *PreprocessedEntityOffsets = nullptr;
   unsigned NumPreprocessedEntities = 0;
 
+  /// \brief Base ID for preprocessed skipped ranges local to this module.
+  unsigned BasePreprocessedSkippedRangeID = 0;
+
+  const PPSkippedRange *PreprocessedSkippedRangeOffsets = nullptr;
+  unsigned NumPreprocessedSkippedRanges = 0;
+
   // === Header search information ===
 
   /// \brief The number of local HeaderFileInfo structures.
Index: include/clang/Serialization/ASTBitCodes.h
===
--- include/clang/Serialization/ASTBitCodes.h
+++ include/clang/Serialization/ASTBitCodes.h
@@ -198,6 +198,25 @@
   }
 };
 
+/// \brief Source range of a skipped preprocessor region
+struct PPSkippedRange {
+  /// \brief Raw source location of beginning of range.
+  unsigned Begin;
+  /// \brief Raw source location of end of range.
+  unsigned End;
+
+  PPSkippedRange(SourceRange R)
+: Begin(R.getBegin().getRawEncoding()),
+  End(R.getEnd().getRawEncoding()) { }
+
+  SourceLocation getBegin() const {
+return SourceLocation::getFromRawEncoding(Begin);
+  }
+  SourceLocation getEnd() const {
+return SourceLocation::getFromRawEncoding(End);
+  }
+};
+
 /// \brief Source range/offset of a preprocessed entity.
 struct DeclOffset {
   /// \brief Raw source location.
@@ -627,6 +646,9 @@
 
   /// \brief The stack of open #ifs/#ifdefs recorded in a preamble.
   PP_CONDITIONAL_STACK = 62,
+
+  /// \brief A table of skipped ranges within the preprocessing record.
+  PPD_SKIPPED_RANGES = 63
 };
 
 /// \brief Record types used within a source manager block.
Index: include/clang/Serialization/ASTReader.h
===
--- include/clang/Serialization/ASTReader.h
+++ include/clang/Serialization/ASTReader.h
@@ -753,6 +753,13 @@
   /// added to the global preprocessing entity ID to produce a local ID.
   GlobalPreprocessedEntityMapType GlobalPreprocessedEntityMap;
 
+  

[PATCH] D67037: [ClangFormat] Add new style option IndentGotoLabels

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp updated this revision to Diff 219582.
tetsuo-cpp added a comment.

Addressed review comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67037

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1408,6 +1408,30 @@
"test_label:;\n"
"  int i = 0;\n"
"}");
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentGotoLabels = false;
+  verifyFormat("void f() {\n"
+   "  some_code();\n"
+   "test_label:\n"
+   "  some_other_code();\n"
+   "  {\n"
+   "some_more_code();\n"
+   "another_label:\n"
+   "some_more_code();\n"
+   "  }\n"
+   "}",
+   Style);
+  verifyFormat("{\n"
+   "  some_code();\n"
+   "test_label:\n"
+   "  some_other_code();\n"
+   "}",
+   Style);
+  verifyFormat("{\n"
+   "  some_code();\n"
+   "test_label:;\n"
+   "  int i = 0;\n"
+   "}");
 }
 
 //===--===//
@@ -11769,6 +11793,7 @@
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
+  CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -106,7 +106,7 @@
   void parseTryCatch();
   void parseForOrWhileLoop();
   void parseDoWhile();
-  void parseLabel();
+  void parseLabel(bool LeftAlignLabel = false);
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1351,7 +1351,7 @@
   (TokenCount == 2 && Line->Tokens.front().Tok->is(tok::comment))) {
 if (FormatTok->Tok.is(tok::colon) && !Line->MustBeDeclaration) {
   Line->Tokens.begin()->Tok->MustBreakBefore = true;
-  parseLabel();
+  parseLabel(!Style.IndentGotoLabels);
   return;
 }
 // Recognize function-like macro usages without trailing semicolon as
@@ -1970,11 +1970,13 @@
   parseStructuralElement();
 }
 
-void UnwrappedLineParser::parseLabel() {
+void UnwrappedLineParser::parseLabel(bool LeftAlignLabel) {
   nextToken();
   unsigned OldLineLevel = Line->Level;
   if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0))
 --Line->Level;
+  if (LeftAlignLabel)
+Line->Level = 0;
   if (CommentsBeforeNextToken.empty() && FormatTok->Tok.is(tok::l_brace)) {
 CompoundStatementIndenter Indenter(this, Line->Level,
Style.BraceWrapping.AfterCaseLabel,
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -453,6 +453,7 @@
 IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
+IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
 IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionNames",
@@ -725,6 +726,7 @@
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
   LLVMStyle.IndentCaseLabels = false;
+  LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1265,6 +1265,22 @@
   /// \endcode
   bool IndentCaseLabels;
 
+  /// Indent goto labels.
+  ///
+  /// When ``false``, goto labels are flushed left.
+  /// \code
+  ///   

[PATCH] D67037: [ClangFormat] Add new style option IndentGotoLabels

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp added a comment.

@MyDeveloperDay
Thanks for your suggestions! I think it's better now.
This is my first LLVM patch (hopefully first of many) so I don't have commit 
permissions yet. If you're still happy with this change, could you please 
commit on my behalf?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67037



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

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp added a comment.

Friendly ping. Could I please get this looked at?


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

https://reviews.llvm.org/D83188



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

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp updated this revision to Diff 280082.
tetsuo-cpp added a comment.

Cleanup check conditional.


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

https://reviews.llvm.org/D83188

Files:
  clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
@@ -74,9 +74,31 @@
 bool *b;
   } d = { SomeFunction() };
 
-  if (d.b)
+  if (d.b) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (*d.b) {
+  }
+
+  if (d.b) {
 (void)*d.b; // no-warning
+  }
 
-#define CHECK(b) if (b) {}
+#define CHECK(b) \
+  if (b) {   \
+  }
   CHECK(c)
 }
+
+struct H {
+  bool *b;
+  void foo() const {
+if (b) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: dubious check of 'bool *' against 'nullptr'
+  // CHECK-FIXES: if (*b) {
+}
+
+if (b) {
+  (void)*b; // no-warning
+}
+  }
+};
Index: clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
@@ -20,53 +20,68 @@
   Finder->addMatcher(
   traverse(
   ast_type_traits::TK_AsIs,
-  ifStmt(hasCondition(findAll(implicitCastExpr(
- unless(hasParent(unaryOperator(hasOperatorName("!",
- hasSourceExpression(expr(
- hasType(pointerType(pointee(booleanType(,
- ignoringParenImpCasts(declRefExpr().bind("expr",
- hasCastKind(CK_PointerToBoolean,
- unless(isInTemplateInstantiation()))
+  ifStmt(
+  hasCondition(findAll(implicitCastExpr(
+  unless(hasParent(unaryOperator(hasOperatorName("!",
+  hasSourceExpression(expr(
+  hasType(pointerType(pointee(booleanType(,
+  ignoringParenImpCasts(anyOf(declRefExpr().bind("expr"),
+  memberExpr().bind("expr"),
+  hasCastKind(CK_PointerToBoolean,
+  unless(isInTemplateInstantiation()))
   .bind("if")),
   this);
 }
 
-void BoolPointerImplicitConversionCheck::check(
-const MatchFinder::MatchResult &Result) {
-  auto *If = Result.Nodes.getNodeAs("if");
-  auto *Var = Result.Nodes.getNodeAs("expr");
-
+static void checkImpl(const MatchFinder::MatchResult &Result, const Expr *Ref,
+  const IfStmt *If,
+  const ast_matchers::internal::Matcher &RefMatcher,
+  ClangTidyCheck &Check) {
   // Ignore macros.
-  if (Var->getBeginLoc().isMacroID())
+  if (Ref->getBeginLoc().isMacroID())
 return;
 
-  // Only allow variable accesses for now, no function calls or member exprs.
+  // Only allow variable accesses and member exprs for now, no function calls.
   // Check that we don't dereference the variable anywhere within the if. This
   // avoids false positives for checks of the pointer for nullptr before it is
   // dereferenced. If there is a dereferencing operator on this variable don't
   // emit a diagnostic. Also ignore array subscripts.
-  const Decl *D = Var->getDecl();
-  auto DeclRef = ignoringParenImpCasts(declRefExpr(to(equalsNode(D;
-  if (!match(findAll(
- unaryOperator(hasOperatorName("*"), hasUnaryOperand(DeclRef))),
+  if (!match(findAll(unaryOperator(hasOperatorName("*"),
+   hasUnaryOperand(RefMatcher))),
  *If, *Result.Context)
.empty() ||
-  !match(findAll(arraySubscriptExpr(hasBase(DeclRef))), *If,
+  !match(findAll(arraySubscriptExpr(hasBase(RefMatcher))), *If,
  *Result.Context)
.empty() ||
   // FIXME: We should still warn if the paremater is implicitly converted to
   // bool.
-  !match(findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(DeclRef,
- *If, *Result.Context)
+  !match(
+   findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(RefMatcher,
+   *If, *Result.Context)
.empty() ||
-  !match(findAll(cxxDeleteExpr(has(ignoringParenImpCasts(expr(DeclRef),
- *If, *Result.Context)
+  !match(
+   findAll(cxxDeleteExpr(has(ignoringParenImpCasts(expr(RefMatcher),
+   *If, *Result.Contex

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp marked an inline comment as done.
tetsuo-cpp added a comment.

@aaron.ballman 
Thanks! I took your suggestion and I think this looks cleaner.


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

https://reviews.llvm.org/D83188



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

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp created this revision.
tetsuo-cpp added reviewers: njames93, MaskRay.
tetsuo-cpp added projects: clang, clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=45189
This patch is adding support for members to the 
`bugprone-bool-pointer-implicit-conversion` check. Apologies if I've done this 
in a weird/wrong way, I'm still getting my head around the matching DSL.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83188

Files:
  clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
@@ -70,13 +70,72 @@
 *(c) = false; // no-warning
   }
 
+#define CHECK(b) \
+  if (b) {   \
+  }
+  CHECK(c)
+
   struct {
 bool *b;
   } d = { SomeFunction() };
 
-  if (d.b)
-(void)*d.b; // no-warning
+  if (d.b) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (*d.b) {
+  }
 
-#define CHECK(b) if (b) {}
-  CHECK(c)
+  if (F() && d.b) {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (F() && *d.b) {
+  }
+
+  // TODO: warn here.
+  if (d.b) {
+G(d.b);
+  }
+
+#define TESTMACRO2 if (d.b || F())
+
+  TESTMACRO2 {
+  }
+
+  t(d.b);
+
+  if (!d.b) {
+// no-warning
+  }
+
+  if (d.b) {
+*d.b = true; // no-warning
+  }
+
+  if (d.b) {
+d.b[0] = false; // no-warning
+  }
+
+  if (d.b) {
+SomeOtherFunction(d.b); // no-warning
+  }
+
+  if (d.b) {
+delete[] d.b; // no-warning
+  }
+
+  if (d.b) {
+*(d.b) = false; // no-warning
+  }
 }
+
+struct H {
+  bool *b;
+  void foo() const {
+if (b) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: dubious check of 'bool *' against 'nullptr'
+  // CHECK-FIXES: if (*b) {
+}
+
+if (b) {
+  (void)*b; // no-warning
+}
+  }
+};
Index: clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
@@ -20,33 +20,33 @@
   Finder->addMatcher(
   traverse(
   ast_type_traits::TK_AsIs,
-  ifStmt(hasCondition(findAll(implicitCastExpr(
- unless(hasParent(unaryOperator(hasOperatorName("!",
- hasSourceExpression(expr(
- hasType(pointerType(pointee(booleanType(,
- ignoringParenImpCasts(declRefExpr().bind("expr",
- hasCastKind(CK_PointerToBoolean,
- unless(isInTemplateInstantiation()))
+  ifStmt(
+  hasCondition(findAll(implicitCastExpr(
+  unless(hasParent(unaryOperator(hasOperatorName("!",
+  hasSourceExpression(expr(
+  hasType(pointerType(pointee(booleanType(,
+  ignoringParenImpCasts(anyOf(declRefExpr().bind("expr"),
+  memberExpr().bind("expr"),
+  hasCastKind(CK_PointerToBoolean,
+  unless(isInTemplateInstantiation()))
   .bind("if")),
   this);
 }
 
-void BoolPointerImplicitConversionCheck::check(
-const MatchFinder::MatchResult &Result) {
-  auto *If = Result.Nodes.getNodeAs("if");
-  auto *Var = Result.Nodes.getNodeAs("expr");
-
+template 
+static void checkImpl(const MatchFinder::MatchResult &Result,
+  const RefExpr *Var, const IfStmt *If,
+  const ast_matchers::internal::Matcher &DeclRef,
+  ClangTidyCheck &Check) {
   // Ignore macros.
   if (Var->getBeginLoc().isMacroID())
 return;
 
-  // Only allow variable accesses for now, no function calls or member exprs.
+  // Only allow variable accesses and member exprs for now, no function calls.
   // Check that we don't dereference the variable anywhere within the if. This
   // avoids false positives for checks of the pointer for nullptr before it is
   // dereferenced. If there is a dereferencing operator on this variable don't
   // emit a diagnostic. Also ignore array subscripts.
-  const Decl *D = Var->getDecl();
-  auto DeclRef = ignoringParenImpCasts(declRefExpr(to(equalsNode(D;
   if (!match(findAll(
  unaryOperator(hasOperatorName("*"), hasUnaryOperand(DeclRef))),
  *If, *Result.Context)
@@ -64,11 +6

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp updated this revision to Diff 275592.
tetsuo-cpp added a comment.

Remove the unnecessary template.


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

https://reviews.llvm.org/D83188

Files:
  clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
@@ -70,13 +70,72 @@
 *(c) = false; // no-warning
   }
 
+#define CHECK(b) \
+  if (b) {   \
+  }
+  CHECK(c)
+
   struct {
 bool *b;
   } d = { SomeFunction() };
 
-  if (d.b)
-(void)*d.b; // no-warning
+  if (d.b) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (*d.b) {
+  }
 
-#define CHECK(b) if (b) {}
-  CHECK(c)
+  if (F() && d.b) {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (F() && *d.b) {
+  }
+
+  // TODO: warn here.
+  if (d.b) {
+G(d.b);
+  }
+
+#define TESTMACRO2 if (d.b || F())
+
+  TESTMACRO2 {
+  }
+
+  t(d.b);
+
+  if (!d.b) {
+// no-warning
+  }
+
+  if (d.b) {
+*d.b = true; // no-warning
+  }
+
+  if (d.b) {
+d.b[0] = false; // no-warning
+  }
+
+  if (d.b) {
+SomeOtherFunction(d.b); // no-warning
+  }
+
+  if (d.b) {
+delete[] d.b; // no-warning
+  }
+
+  if (d.b) {
+*(d.b) = false; // no-warning
+  }
 }
+
+struct H {
+  bool *b;
+  void foo() const {
+if (b) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: dubious check of 'bool *' against 'nullptr'
+  // CHECK-FIXES: if (*b) {
+}
+
+if (b) {
+  (void)*b; // no-warning
+}
+  }
+};
Index: clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
@@ -20,33 +20,32 @@
   Finder->addMatcher(
   traverse(
   ast_type_traits::TK_AsIs,
-  ifStmt(hasCondition(findAll(implicitCastExpr(
- unless(hasParent(unaryOperator(hasOperatorName("!",
- hasSourceExpression(expr(
- hasType(pointerType(pointee(booleanType(,
- ignoringParenImpCasts(declRefExpr().bind("expr",
- hasCastKind(CK_PointerToBoolean,
- unless(isInTemplateInstantiation()))
+  ifStmt(
+  hasCondition(findAll(implicitCastExpr(
+  unless(hasParent(unaryOperator(hasOperatorName("!",
+  hasSourceExpression(expr(
+  hasType(pointerType(pointee(booleanType(,
+  ignoringParenImpCasts(anyOf(declRefExpr().bind("expr"),
+  memberExpr().bind("expr"),
+  hasCastKind(CK_PointerToBoolean,
+  unless(isInTemplateInstantiation()))
   .bind("if")),
   this);
 }
 
-void BoolPointerImplicitConversionCheck::check(
-const MatchFinder::MatchResult &Result) {
-  auto *If = Result.Nodes.getNodeAs("if");
-  auto *Var = Result.Nodes.getNodeAs("expr");
-
+static void checkImpl(const MatchFinder::MatchResult &Result, const Expr *Var,
+  const IfStmt *If,
+  const ast_matchers::internal::Matcher &DeclRef,
+  ClangTidyCheck &Check) {
   // Ignore macros.
   if (Var->getBeginLoc().isMacroID())
 return;
 
-  // Only allow variable accesses for now, no function calls or member exprs.
+  // Only allow variable accesses and member exprs for now, no function calls.
   // Check that we don't dereference the variable anywhere within the if. This
   // avoids false positives for checks of the pointer for nullptr before it is
   // dereferenced. If there is a dereferencing operator on this variable don't
   // emit a diagnostic. Also ignore array subscripts.
-  const Decl *D = Var->getDecl();
-  auto DeclRef = ignoringParenImpCasts(declRefExpr(to(equalsNode(D;
   if (!match(findAll(
  unaryOperator(hasOperatorName("*"), hasUnaryOperand(DeclRef))),
  *If, *Result.Context)
@@ -64,11 +63,30 @@
.empty())
 return;
 
-  diag(Var->getBeginLoc(), "dubious check of 'bool *' against 'nullptr', did "
-   "you mean to dereference it?")
+  Check.diag(Var->getBeginLoc(),
+ "dubious check of 'bool *' against 'nullptr', did "
+ "you mean to derefe

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp updated this revision to Diff 276022.
tetsuo-cpp added a comment.

Address comments: Rename variables, remove unnecessary tests and avoid 
unnecessary check.


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

https://reviews.llvm.org/D83188

Files:
  clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
@@ -74,9 +74,31 @@
 bool *b;
   } d = { SomeFunction() };
 
-  if (d.b)
+  if (d.b) {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (*d.b) {
+  }
+
+  if (d.b) {
 (void)*d.b; // no-warning
+  }
 
-#define CHECK(b) if (b) {}
+#define CHECK(b) \
+  if (b) {   \
+  }
   CHECK(c)
 }
+
+struct H {
+  bool *b;
+  void foo() const {
+if (b) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: dubious check of 'bool *' against 'nullptr'
+  // CHECK-FIXES: if (*b) {
+}
+
+if (b) {
+  (void)*b; // no-warning
+}
+  }
+};
Index: clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
@@ -20,53 +20,73 @@
   Finder->addMatcher(
   traverse(
   ast_type_traits::TK_AsIs,
-  ifStmt(hasCondition(findAll(implicitCastExpr(
- unless(hasParent(unaryOperator(hasOperatorName("!",
- hasSourceExpression(expr(
- hasType(pointerType(pointee(booleanType(,
- ignoringParenImpCasts(declRefExpr().bind("expr",
- hasCastKind(CK_PointerToBoolean,
- unless(isInTemplateInstantiation()))
+  ifStmt(
+  hasCondition(findAll(implicitCastExpr(
+  unless(hasParent(unaryOperator(hasOperatorName("!",
+  hasSourceExpression(expr(
+  hasType(pointerType(pointee(booleanType(,
+  ignoringParenImpCasts(anyOf(declRefExpr().bind("expr"),
+  memberExpr().bind("expr"),
+  hasCastKind(CK_PointerToBoolean,
+  unless(isInTemplateInstantiation()))
   .bind("if")),
   this);
 }
 
-void BoolPointerImplicitConversionCheck::check(
-const MatchFinder::MatchResult &Result) {
-  auto *If = Result.Nodes.getNodeAs("if");
-  auto *Var = Result.Nodes.getNodeAs("expr");
-
+static void checkImpl(const MatchFinder::MatchResult &Result, const Expr *Ref,
+  const IfStmt *If,
+  const ast_matchers::internal::Matcher &RefMatcher,
+  ClangTidyCheck &Check) {
   // Ignore macros.
-  if (Var->getBeginLoc().isMacroID())
+  if (Ref->getBeginLoc().isMacroID())
 return;
 
-  // Only allow variable accesses for now, no function calls or member exprs.
+  // Only allow variable accesses and member exprs for now, no function calls.
   // Check that we don't dereference the variable anywhere within the if. This
   // avoids false positives for checks of the pointer for nullptr before it is
   // dereferenced. If there is a dereferencing operator on this variable don't
   // emit a diagnostic. Also ignore array subscripts.
-  const Decl *D = Var->getDecl();
-  auto DeclRef = ignoringParenImpCasts(declRefExpr(to(equalsNode(D;
-  if (!match(findAll(
- unaryOperator(hasOperatorName("*"), hasUnaryOperand(DeclRef))),
+  if (!match(findAll(unaryOperator(hasOperatorName("*"),
+   hasUnaryOperand(RefMatcher))),
  *If, *Result.Context)
.empty() ||
-  !match(findAll(arraySubscriptExpr(hasBase(DeclRef))), *If,
+  !match(findAll(arraySubscriptExpr(hasBase(RefMatcher))), *If,
  *Result.Context)
.empty() ||
   // FIXME: We should still warn if the paremater is implicitly converted to
   // bool.
-  !match(findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(DeclRef,
- *If, *Result.Context)
+  !match(
+   findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(RefMatcher,
+   *If, *Result.Context)
.empty() ||
-  !match(findAll(cxxDeleteExpr(has(ignoringParenImpCasts(expr(DeclRef),
- *If, *Result.Context)
+  !match(
+   findAll(cxxDeleteExpr(has(ignoringPar

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp added a comment.

Friendly ping!


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

https://reviews.llvm.org/D83188



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

[PATCH] D83188: [clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp added a comment.

Friendly ping @njames93.
Does this look ok to you? If so, could you please help me with the commit?


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

https://reviews.llvm.org/D83188

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

[PATCH] D67037: [ClangFormat] Add new style option IndentGotoLabels

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp created this revision.
tetsuo-cpp added a reviewer: klimek.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This option determines whether goto labels are indented according to scope. 
Setting this option to false causes goto labels to be flushed to the left.
This is mostly copied from this patch 
 submitted 
by Christian Neukirchen that didn't make its way into trunk.

  true:  false:
  int f() {  vs. int f() {
if (foo()) {   if (foo()) {
label1:  label1:
  bar(); bar();
}  }
  label2:label2:
return 1;  return 1;
  }  }


Repository:
  rC Clang

https://reviews.llvm.org/D67037

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1408,6 +1408,30 @@
"test_label:;\n"
"  int i = 0;\n"
"}");
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentGotoLabels = false;
+  verifyFormat("void f() {\n"
+   "  some_code();\n"
+   "test_label:\n"
+   "  some_other_code();\n"
+   "  {\n"
+   "some_more_code();\n"
+   "another_label:\n"
+   "some_more_code();\n"
+   "  }\n"
+   "}",
+   Style);
+  verifyFormat("{\n"
+   "  some_code();\n"
+   "test_label:\n"
+   "  some_other_code();\n"
+   "}",
+   Style);
+  verifyFormat("{\n"
+   "  some_code();\n"
+   "test_label:;\n"
+   "  int i = 0;\n"
+   "}");
 }
 
 //===--===//
@@ -11769,6 +11793,7 @@
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
+  CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -106,7 +106,7 @@
   void parseTryCatch();
   void parseForOrWhileLoop();
   void parseDoWhile();
-  void parseLabel();
+  void parseLabel(bool GotoLabel = false);
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1351,7 +1351,7 @@
   (TokenCount == 2 && Line->Tokens.front().Tok->is(tok::comment))) {
 if (FormatTok->Tok.is(tok::colon) && !Line->MustBeDeclaration) {
   Line->Tokens.begin()->Tok->MustBreakBefore = true;
-  parseLabel();
+  parseLabel(true);
   return;
 }
 // Recognize function-like macro usages without trailing semicolon as
@@ -1970,11 +1970,13 @@
   parseStructuralElement();
 }
 
-void UnwrappedLineParser::parseLabel() {
+void UnwrappedLineParser::parseLabel(bool GotoLabel) {
   nextToken();
   unsigned OldLineLevel = Line->Level;
   if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0))
 --Line->Level;
+  if (GotoLabel && !Style.IndentGotoLabels)
+Line->Level = 0;
   if (CommentsBeforeNextToken.empty() && FormatTok->Tok.is(tok::l_brace)) {
 CompoundStatementIndenter Indenter(this, Line->Level,
Style.BraceWrapping.AfterCaseLabel,
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -453,6 +453,7 @@
 IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
 IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
+IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
 IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
 IO.mapOptional("IndentWidth", Style.IndentWidth);
 IO.mapOptional("IndentWrappedFunctionName

[PATCH] D67037: [ClangFormat] Add new style option IndentGotoLabels

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp added a comment.

Friendly ping. Could I please get this looked at?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67037



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

[PATCH] D157990: [clangd] Add --query-driver flag to clangd-indexer

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp created this revision.
tetsuo-cpp added reviewers: sammccall, nridge.
tetsuo-cpp added projects: clang-tools-extra, All.
Herald added subscribers: kadircet, arphaman.
tetsuo-cpp requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

When using `clangd` for cross-compiled projects, it's necessary to use the 
`--query-driver` flag so that `clangd` can extract the compiler's built-in 
header paths. However, there's no such flag for `clangd-indexer` so we're 
unable to build a working static index for these projects.

This patch adds a `--query-driver` flag to `clangd-indexer` for this scenario.

I saw some tests under `clang-tools-extra/clangd/test/` but I think the 
cross-compilation case is a bit more complex to test. Let me know if you'd like 
me to look into this further.

Resolves: https://github.com/clangd/clangd/issues/1717


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157990

Files:
  clang-tools-extra/clangd/indexer/IndexerMain.cpp


Index: clang-tools-extra/clangd/indexer/IndexerMain.cpp
===
--- clang-tools-extra/clangd/indexer/IndexerMain.cpp
+++ clang-tools-extra/clangd/indexer/IndexerMain.cpp
@@ -38,6 +38,16 @@
"binary RIFF format")),
llvm::cl::init(IndexFileFormat::RIFF));
 
+static llvm::cl::list QueryDriverGlobs{
+"query-driver",
+llvm::cl::desc(
+"Comma separated list of globs for white-listing gcc-compatible "
+"drivers that are safe to execute. Drivers matching any of these globs 
"
+"will be used to extract system includes. e.g. "
+"/usr/bin/**/clang-*,/path/to/repo/**/g++-*"),
+llvm::cl::CommaSeparated,
+};
+
 class IndexActionFactory : public tooling::FrontendActionFactory {
 public:
   IndexActionFactory(IndexFileIn &Result) : Result(Result) {}
@@ -144,12 +154,16 @@
 
   // Collect symbols found in each translation unit, merging as we go.
   clang::clangd::IndexFileIn Data;
+  auto Mangler = std::make_shared(
+  clang::clangd::CommandMangler::detect());
+  Mangler->SystemIncludeExtractor = clang::clangd::getSystemIncludeExtractor(
+  static_cast>(
+  clang::clangd::QueryDriverGlobs));
   auto Err = Executor->get()->execute(
   std::make_unique(Data),
   clang::tooling::ArgumentsAdjuster(
-  [Mangler = std::make_shared(
-   clang::clangd::CommandMangler::detect())](
-  const std::vector &Args, llvm::StringRef File) {
+  [Mangler = std::move(Mangler)](const std::vector &Args,
+ llvm::StringRef File) {
 clang::tooling::CompileCommand Cmd;
 Cmd.CommandLine = Args;
 Mangler->operator()(Cmd, File);


Index: clang-tools-extra/clangd/indexer/IndexerMain.cpp
===
--- clang-tools-extra/clangd/indexer/IndexerMain.cpp
+++ clang-tools-extra/clangd/indexer/IndexerMain.cpp
@@ -38,6 +38,16 @@
"binary RIFF format")),
llvm::cl::init(IndexFileFormat::RIFF));
 
+static llvm::cl::list QueryDriverGlobs{
+"query-driver",
+llvm::cl::desc(
+"Comma separated list of globs for white-listing gcc-compatible "
+"drivers that are safe to execute. Drivers matching any of these globs "
+"will be used to extract system includes. e.g. "
+"/usr/bin/**/clang-*,/path/to/repo/**/g++-*"),
+llvm::cl::CommaSeparated,
+};
+
 class IndexActionFactory : public tooling::FrontendActionFactory {
 public:
   IndexActionFactory(IndexFileIn &Result) : Result(Result) {}
@@ -144,12 +154,16 @@
 
   // Collect symbols found in each translation unit, merging as we go.
   clang::clangd::IndexFileIn Data;
+  auto Mangler = std::make_shared(
+  clang::clangd::CommandMangler::detect());
+  Mangler->SystemIncludeExtractor = clang::clangd::getSystemIncludeExtractor(
+  static_cast>(
+  clang::clangd::QueryDriverGlobs));
   auto Err = Executor->get()->execute(
   std::make_unique(Data),
   clang::tooling::ArgumentsAdjuster(
-  [Mangler = std::make_shared(
-   clang::clangd::CommandMangler::detect())](
-  const std::vector &Args, llvm::StringRef File) {
+  [Mangler = std::move(Mangler)](const std::vector &Args,
+ llvm::StringRef File) {
 clang::tooling::CompileCommand Cmd;
 Cmd.CommandLine = Args;
 Mangler->operator()(Cmd, File);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157990: [clangd] Add --query-driver flag to clangd-indexer

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp added a comment.

Thanks for the review @nridge! I'd really appreciate if you could help me with 
the commit (I don't have commit access).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157990

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

[PATCH] D75901: [clang-tidy] misc-unconventional-assign-operator suggest to use rvalue references in C++03 mode

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp created this revision.
tetsuo-cpp added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
tetsuo-cpp added reviewers: njames93, MaskRay.

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=27702
I wasn't sure how this type of thing is usually tested. So any advice would be 
appreciated.
`check-llvm`, `check-clang` and `check-clang-tools` are clean for me.
**C++98**

  tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ cat compile_commands.json
  [
  {
"directory": "/home/tetsuo/dev/llvm-project/test",
"command": "/usr/bin/c++  -std=gnu++98 -o 
CMakeFiles/test.dir/test.cpp.o -c /home/tetsuo/dev/llvm-project/test/test.cpp",
"file": "/home/tetsuo/dev/llvm-project/test/test.cpp"
  }
  ]
  tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ ../build/bin/clang-tidy 
--checks=misc-unconventional-assign-operator test.cpp
  3053 warnings generated.
  /home/tetsuo/dev/llvm-project/test/test.cpp:7:3: warning: operator=() should 
take 'Foo const&' or 'Foo' [misc-unconventional-assign-operator]
Foo &operator=(Foo &Other) {
^
  Suppressed 3052 warnings (3052 in non-user code).
  Use -header-filter=.* to display errors from all non-system headers. Use 
-system-headers to display errors from system headers as well.

**C++17**

  tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ cat compile_commands.json
  [
  {
"directory": "/home/tetsuo/dev/llvm-project/test",
"command": "/usr/bin/c++  -std=gnu++17 -o 
CMakeFiles/test.dir/test.cpp.o -c /home/tetsuo/dev/llvm-project/test/test.cpp",
"file": "/home/tetsuo/dev/llvm-project/test/test.cpp"
  }
  ]
  tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ ../build/bin/clang-tidy 
--checks=misc-unconventional-assign-operator test.cpp
  5377 warnings generated.
  /home/tetsuo/dev/llvm-project/test/test.cpp:7:3: warning: operator=() should 
take 'Foo const&', 'Foo&&' or 'Foo' [misc-unconventional-assign-operator]
Foo &operator=(Foo &Other) {
^
  Suppressed 5376 warnings (5376 in non-user code).
  Use -header-filter=.* to display errors from all non-system headers. Use 
-system-headers to display errors from system headers as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75901

Files:
  clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp


Index: clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
@@ -73,9 +73,14 @@
   if (const auto *RetStmt = Result.Nodes.getNodeAs("returnStmt")) {
 diag(RetStmt->getBeginLoc(), "operator=() should always return '*this'");
   } else {
+const ASTContext *ASTCtx = Result.Context;
+const LangOptions &Opts = ASTCtx->getLangOpts();
 static const char *const Messages[][2] = {
 {"ReturnType", "operator=() should return '%0&'"},
-{"ArgumentType", "operator=() should take '%0 const&', '%0&&' or 
'%0'"},
+{"ArgumentType",
+ Opts.CPlusPlus11
+ ? "operator=() should take '%0 const&', '%0&&' or '%0'"
+ : "operator=() should take '%0 const&' or '%0'"},
 {"cv", "operator=() should not be marked '%1'"}};
 
 const auto *Method = Result.Nodes.getNodeAs("method");


Index: clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
@@ -73,9 +73,14 @@
   if (const auto *RetStmt = Result.Nodes.getNodeAs("returnStmt")) {
 diag(RetStmt->getBeginLoc(), "operator=() should always return '*this'");
   } else {
+const ASTContext *ASTCtx = Result.Context;
+const LangOptions &Opts = ASTCtx->getLangOpts();
 static const char *const Messages[][2] = {
 {"ReturnType", "operator=() should return '%0&'"},
-{"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
+{"ArgumentType",
+ Opts.CPlusPlus11
+ ? "operator=() should take '%0 const&', '%0&&' or '%0'"
+ : "operator=() should take '%0 const&' or '%0'"},
 {"cv", "operator=() should not be marked '%1'"}};
 
 const auto *Method = Result.Nodes.getNodeAs("method");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75901: [clang-tidy] misc-unconventional-assign-operator suggest to use rvalue references in C++03 mode

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp updated this revision to Diff 249538.
tetsuo-cpp added a comment.

Address review comments.


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

https://reviews.llvm.org/D75901

Files:
  clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator-precxx11.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator-precxx11.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator-precxx11.cpp
@@ -0,0 +1,6 @@
+// RUN: %check_clang_tidy -std=c++98,c++03 %s 
misc-unconventional-assign-operator %t
+
+struct BadArgument {
+  BadArgument &operator=(BadArgument &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 
'BadArgument const&' or 'BadArgument'
+};
Index: clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
@@ -75,7 +75,10 @@
   } else {
 static const char *const Messages[][2] = {
 {"ReturnType", "operator=() should return '%0&'"},
-{"ArgumentType", "operator=() should take '%0 const&', '%0&&' or 
'%0'"},
+{"ArgumentType",
+ getLangOpts().CPlusPlus11
+ ? "operator=() should take '%0 const&', '%0&&' or '%0'"
+ : "operator=() should take '%0 const&' or '%0'"},
 {"cv", "operator=() should not be marked '%1'"}};
 
 const auto *Method = Result.Nodes.getNodeAs("method");


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator-precxx11.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator-precxx11.cpp
@@ -0,0 +1,6 @@
+// RUN: %check_clang_tidy -std=c++98,c++03 %s misc-unconventional-assign-operator %t
+
+struct BadArgument {
+  BadArgument &operator=(BadArgument &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&' or 'BadArgument'
+};
Index: clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
@@ -75,7 +75,10 @@
   } else {
 static const char *const Messages[][2] = {
 {"ReturnType", "operator=() should return '%0&'"},
-{"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
+{"ArgumentType",
+ getLangOpts().CPlusPlus11
+ ? "operator=() should take '%0 const&', '%0&&' or '%0'"
+ : "operator=() should take '%0 const&' or '%0'"},
 {"cv", "operator=() should not be marked '%1'"}};
 
 const auto *Method = Result.Nodes.getNodeAs("method");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75901: [clang-tidy] misc-unconventional-assign-operator suggest to use rvalue references in C++03 mode

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp marked an inline comment as done.
tetsuo-cpp added a comment.

@njames93 @MaskRay 
Thanks for helping me with testing. I'll remember this for next time.

I also saw this pattern of retrieving `LangOptions` from an `ASTContext` in 
some other checks (`RedundantExpressionCheck` and `StaticAssertCheck`) but I 
didn't change them to use `getLangOpts` since the `ASTContext` is used for 
other stuff.


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

https://reviews.llvm.org/D75901



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

[PATCH] D75901: [clang-tidy] misc-unconventional-assign-operator suggest to use rvalue references in C++03 mode

[Alex Cameron via Phabricator via cfe-commits]

tetsuo-cpp added a comment.

In D75901#1920397 , @njames93 wrote:

> In D75901#1916119 , @tetsuo-cpp 
> wrote:
>
> > @njames93 @MaskRay 
> >  Thanks for helping me with testing. I'll remember this for next time.
> >
> > I also saw this pattern of retrieving `LangOptions` from an `ASTContext` in 
> > some other checks (`RedundantExpressionCheck` and `StaticAssertCheck`) but 
> > I didn't change them to use `getLangOpts` since the `ASTContext` is used 
> > for other stuff.
>
>
> Its also not a good idea to change unrelated code in a review


Understood. Thanks @njames93.
Could you please help me commit this change?


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

https://reviews.llvm.org/D75901



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