[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-12-05 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik marked 2 inline comments as done. nik added inline comments. Comment at: include/clang/Lex/Preprocessor.h:391 } PreambleConditionalStack; + bool PreambleGenerationFailed = false; ilya-biryukov wrote: > There's a mechanism to handle preamble with errors,

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-12-05 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik updated this revision to Diff 176826. nik added a comment. Addressed comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 Files: include/clang/Lex/Preprocessor.h include/clang/Serialization/ASTWriter.h lib/

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-12-04 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: include/clang/Lex/Preprocessor.h:391 } PreambleConditionalStack; + bool PreambleGenerationFailed = false; There's a mechanism to handle preamble with errors, see `PreprocessorOpts::AllowPCHWithCompilerErrors

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-12-03 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53866/new/ https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-11-26 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik updated this revision to Diff 175210. nik added a comment. > Maybe produce a **fatal** error in the preprocessor? That seems to be the > simplest option: the preprocessor is aware it's building the preamble and > there's definitely some logic to produce fatal errors in other cases (include

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-11-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. In https://reviews.llvm.org/D53866#1301086, @nik wrote: > I still don't have feedback for a real world case except "unintentional > #include". Unfortunately, in real world cases the cyclic include might be not > obvious at all. > @ilya: As far as I understand you

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-11-16 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik added a comment. I still don't have feedback for a real world case except "unintentional #include". Unfortunately, in real world cases the cyclic include might be not obvious at all. @ilya: As far as I understand you prefer to make the preamble generation rather fail as long as we don't ha

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-11-01 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik added a comment. I've only the minimal example at hand right know - I'm waiting for feedback about the real world case. Repository: rC Clang https://reviews.llvm.org/D53866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-10-31 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. In https://reviews.llvm.org/D53866#1282582, @yvvan wrote: > @ilya-biryukov > As far as I understand the problem the same thing happens when you are in > the header a.h which includes b.h and b.h includes a.h at the same time. So > you get this recursion indirect

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-10-31 Thread Ivan Donchevskii via Phabricator via cfe-commits
yvvan added a comment. @ilya-biryukov As far as I understand the problem the same thing happens when you are in the header a.h which includes b.h and b.h includes a.h at the same time. So you get this recursion indirectly and very often because that's why include guards are there. Repository

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-10-31 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. > Because this case can be detected and handled without loosing the benefits of > the preamble. The cases where recursive includes make sense are incredibly rare in practice. Most of the time, recursively including the same file is unintentional should be conside

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-10-31 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik added a comment. In https://reviews.llvm.org/D53866#1281978, @ilya-biryukov wrote: > Why does resetting the conditional stack is the right thing to do here? Because this case can be detected and handled without loosing the benefits of the preamble. > Failing to build the preamble in that

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-10-31 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Why does resetting the conditional stack is the right thing to do here? I can see how it can hide the problem, but can't come up with with a consistent model to handle the fact that the file contents were trimmed. Failing to build the preamble in that case seems li

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

2018-10-30 Thread Nikolai Kosjar via Phabricator via cfe-commits
nik created this revision. Herald added subscribers: cfe-commits, arphaman. If a header file was processed for the second time, we could end up with a wrong conditional stack and skipped ranges: In the particular example, if the header guard is evaluated the second time and it is decided to skip