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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Parsing errors on preamble additions and removals are definitely bad and should be fixed. 
But I would argue that the right approach is to invalidate the preamble and rebuild it on BOM changes.

Current fix in `ASTUnit` just hides an error in the underlying APIs. For example, all other other clients of `PrecompiledPreamble` are still broken.



================
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(
----------------
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.


https://reviews.llvm.org/D37491