[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