>From a cursory investigation of this feature, it looks like it should be straightforward to write a tiny awk script to convert any users of it, which don't use #unassert, to macros:<div><br></div><div>#assert a(b) -> #define a 1 #define a_b 1</div>
<div>#a (inside pp condition) -> defined(a)</div><div>#a(b) (inside pp condition) -> defined(a_b)</div><div><br></div><div>Given that, and your analysis of the costs, I think we would need quite a compelling reason to accept this extension. Unless someone can present a solid argument in favor of this extension, it does not seem valuable to me.<br>
<br><div class="gmail_quote">On Tue, Mar 6, 2012 at 9:21 AM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>Hi all,</div><div><br></div><div>I'd like to get feedback and consensus on whether we should add "GCC Preprocessor Assertions" to clang. This gcc extension has been deprecated since gcc-3, here's a quote from gcc 3.1 docs, (bold added by me):</div>
<div><br></div><div>From <a href="http://gcc.gnu.org/onlinedocs/gcc-3.1/cpp/Assertions.html" target="_blank">http://gcc.gnu.org/onlinedocs/gcc-3.1/cpp/Assertions.html</a></div><div><br></div><div><blockquote type="cite">Assertions are a <b>deprecated</b> alternative to macros in writing conditionals to test what sort of computer or system the compiled program will run on. Assertions are usually predefined, but you can define them with preprocessing directives or command-line options.<br>
<br>Assertions were intended to provide a more systematic way to describe the compiler's target system. However, in practice they are just as unpredictable as the system-specific predefined macros. In addition, they are not part of any standard, and only a few compilers support them. Therefore, the use of assertions is less portable than the use of system-specific predefined macros. <b>We recommend you do not use them at all</b>.</blockquote>
</div><div><br></div><div>Do note that, after checking on <a href="http://ideone.com" target="_blank">http://ideone.com</a>, gcc still supports them on <span style="color:rgb(51,51,51);font-size:13px">gcc-4.3.4.</span></div>
<div><span style="color:rgb(51,51,51);font-size:13px"><br></span></div><div><span style="color:rgb(51,51,51);font-size:13px"><div style="color:rgb(0,0,0);font-size:medium"><span style="color:rgb(51,51,51);font-size:13px">Andrew did great work on implementing the feature; apart from the preprocessor changes, he added support for them in the preprocessing record and serialization/deserialization support in the ASTReader/ASTWriter.</span></div>
</span><span style="color:rgb(51,51,51);font-size:13px">This is a significant amount of code that we have to maintain and make sure it works well with other parts of the codebase, </span><span style="color:rgb(51,51,51);font-size:13px">and I'm worried about the maintenance burden that this deprecated feature will impose, e.g.</span></div>
<div><span style="color:rgb(51,51,51);font-size:13px"><br></span></div><div><span style="color:rgb(51,51,51);font-size:13px">-it reserves 1 bit out of the IdentifierInfo object. There's only 1 bit left currently so afterwards there will be non left to easily use for other purposes, without doubling the </span><span style="color:rgb(51,51,51);font-size:13px">IdentifierInfo object's size.</span></div>
<div><span style="color:rgb(51,51,51);font-size:13px"><br></span></div><div><span style="color:rgb(51,51,51);font-size:13px">-it increases the size of the IdentifierInfo data that are stored in the serialized AST.</span></div>
<div><span style="color:rgb(51,51,51);font-size:13px"><br></span></div><div><span style="color:rgb(51,51,51);font-size:13px">Although the changes are extensive, there may be more that are needed:</span></div><div><span style="color:rgb(51,51,51);font-size:13px"><br>
</span></div><div><span style="color:rgb(51,51,51);font-size:13px">-gcc docs say that you can specify assertions by command-line options; I did not see this implemented in Andrew's patches</span></div><div><span style="color:rgb(51,51,51);font-size:13px"><br>
</span></div><div><span style="color:rgb(51,51,51);font-size:13px">-the feature was not integrated into our PCH validation, to be more specific, validating and giving errors when assertions from PCH clash with predefined assertions or assertions from the command-line</span></div>
<div><span style="color:rgb(51,51,51);font-size:13px"><br></span></div><div><span style="color:rgb(51,51,51);font-size:13px">-It is not clear to me how this feature should interact with modules.</span></div><div><font color="#333333"><span style="font-size:13px"><br>
</span></font></div><div><font color="#333333"><span style="font-size:13px"><br></span></font></div><div><font color="#333333"><span style="font-size:13px">So, to recap, the question is this, should we implement and maintain a gcc extension that was deprecated since gcc 3 ?</span></font></div>
<div><font color="#333333"><span style="font-size:13px"><br></span></font></div><div><font color="#333333"><span style="font-size:13px">-Argyrios</span></font></div><div><font color="#333333"><span style="font-size:13px"><br>
</span></font></div><div><font color="#333333"><span style="font-size:13px"><br></span></font></div><div><div>On Dec 14, 2011, at 9:26 PM, Andrew Craik wrote:</div><br><blockquote type="cite"><div>Hi Doug,<br><br>Please find attached a series of 4 patches which have incorporated your feedback on my previous attempt to implement support for GCC-style pre-processor assertions in Clang; the feature is now working with PCH and PCH chaining. The patches are as follows from first to last:<br>
1) boost_usuallytinyptrvector_to_basic.patch - moves the UsuallyPtrVector type to Basic, per your previous, and adds an erase method to it<br>2) pp_assert_impl.patch - reworked version of the original patch to implement handling of asserts in the pre-processor<br>
3) pp_assert_defaults.patch - same as previous post adding the default asserts for systems and cpus<br>4) pp_assert_serialization.patch - adds serialization support for assertions and all the required callbacks etc<br><br>
For the serialization I had to add a bit to the IdentifierInfo to track if the identifier had asserted values to facilitate separate deserialization of asserts and macros. This resulted in the size of the emitted IdentifierInfo exceeding the 2 bytes that were allocated for it so I have increased the size as you can see in the patch.<br>
<br>I look forward to hearing from you.<br><br>Andrew<br><br>On 04-Dec-11 08:33, Andrew Craik wrote:<br><blockquote type="cite">Hi Doug,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Thanks for the detailed feedback on the code style and control flow; all<br>
</blockquote><blockquote type="cite">your comments seem very reasonable and I will go and rework the patch. I<br></blockquote><blockquote type="cite">will see if I can add another patch to do the<br></blockquote><blockquote type="cite">
serialization/de-serialization.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Andrew<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">On 04-Dec-11 06:32, Douglas Gregor wrote:<br>
</blockquote><blockquote type="cite"><blockquote type="cite">On Dec 1, 2011, at 11:46 PM, Andrew Craik wrote:<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><blockquote type="cite">Hello,<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><blockquote type="cite">At the LLVM Developers' Meeting a couple of weeks ago I mentioned that I had a patch to add support to the pre-processor to handle GCC-style pre-processor assertions.<br>
</blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">Please find attached two patches:<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
<blockquote type="cite">pp_assert_impl.patch - this adds handling of assertions to the pre-processor<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">pp_assert_default.patch - this adds default cpu and system assertions to Targets.cpp<br>
</blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">
I look forward to hearing from the community and hopefully getting these patches integrated.<br></blockquote></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Thanks. A bunch of comments below, but generally things are look very good!<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Starting with pp_assert_impl.patch:<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Index: clang/include/clang/Lex/Preprocessor.h<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
===================================================================<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">--- clang.orig/include/clang/Lex/Preprocessor.h<span style="white-space:pre-wrap"> </span>2011-11-30 11:39:19.000000000 +1000<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+++ clang/include/clang/Lex/Preprocessor.h<span style="white-space:pre-wrap"> </span>2011-12-02 12:23:53.246940900 +1000<br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">@@ -15,6 +15,7 @@<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> #define LLVM_CLANG_LEX_PREPROCESSOR_H<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> #include "clang/Lex/MacroInfo.h"<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+#include "clang/Lex/AssertionInfo.h"<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> #include "clang/Lex/Lexer.h"<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
#include "clang/Lex/PTHLexer.h"<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> #include "clang/Lex/PPCallbacks.h"<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">@@ -240,6 +241,10 @@<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> /// to the actual definition of the macro.<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"> llvm::DenseMap<IdentifierInfo*, MacroInfo*> Macros;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ /// Assertions - For each IdentifierInfo that has appeared as an asserted<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// predicate, we store the values for which it was asserted<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ llvm::DenseMap<IdentifierInfo*, std::vector<AssertionInfo*> > AssertedValues;<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">I'm guessing that the common case is that there is only one asserted value for each assertion, so I suggest replacing the std::vector with a UsuallyTinyPtrVector (which will need to be boosted from AST to Basic).<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">@@ -440,6 +459,30 @@<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"> macro_iterator macro_begin(bool IncludeExternalMacros = true) const;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> macro_iterator macro_end(bool IncludeExternalMacros = true) const;<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// AddAssertionValue - assert the specified identifier to the<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// set of asserted values for the given assertion key<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ void AddAssertionValue(IdentifierInfo *II, AssertionInfo *AI);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// IsAsserted - test if the specified identifier has been asserted as a<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// value of the given assertion key returns true iff it has<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ bool IsAsserted(IdentifierInfo *II, AssertionInfo *AI);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// RemoveAssertionValue - remove the specified identifier from the set of<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// asserted values for the given assertion key<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ void RemoveAssertionValue(IdentifierInfo *II, AssertionInfo *AI);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// RemoveAssertion - remove the specified assertion and all of its asserted<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// values<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ void RemoveAssertion(IdentifierInfo *II);<br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">A quick request here… the LLVM coding standards clarified the naming of functions a few months ago (<a href="http://llvm.org/docs/CodingStandards.html#ll_naming" target="_blank">http://llvm.org/docs/CodingStandards.html#ll_naming</a>), so please name these addAssertionValue, isAsserted, and so on.<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">The same comment applies throughout the patch, because we want new code to follow this convention and existing code to eventually migrate over.<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ void Destory() {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ ValueTokens.clear();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ this->~AssertionInfo();<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
Typo "Destory", propagated throughout.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+/// ReadAssertionPredicate - Lex and validate an assertion predicate, which<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+/// occurs after an #assert or #unassert. This sets the token kind to eod and<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+/// discards the rest of the line if the assert is invalid.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+/// isAssertUnassert is 1 if this is due to an #assert, 2 if #unassert<br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+/// directive, 0 if it is something else<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+void Preprocessor::ReadAssertionPredicate(Token&AssertionPredTok,<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ char isAssertUnassert) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">Please use an enumeration type rather than 0/1/2 here.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
Also, why not indicate failure by returning 'true' and success by returning 'false', as we do elsewhere in Clang?<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+ IdentifierInfo *II = AssertionPredTok.getIdentifierInfo();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (II == 0) {<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ bool Invalid = false;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ std::string Spelling = getSpelling(AssertionPredTok,&Invalid);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (Invalid) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ return;<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ const IdentifierInfo&Info = Identifiers.get(Spelling);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (Info.isCPlusPlusOperatorKeyword()) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ Diag(AssertionPredTok, diag::err_pp_operator_used_as_assert_predicate)<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<< Spelling;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ } else {<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ Diag(AssertionPredTok, diag::err_pp_predicate_not_identifier);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ // Fall through on error.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ } else if (isAssertUnassert&& II->getPPKeywordID() == tok::pp_assert) {<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // Error if using assert as a predicate since using it would require<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ // #assert<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ Diag(AssertionPredTok, diag::err_assert_predicate_name);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ } else {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // Okay, we got a good identifier node. Return it.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ return;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">If you did switch the return value to 'bool', this code could be de-nested and clarified a bit:<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">if (IdentifierInfo *II = AssertionPredTok.getIdentifierInfo()) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> if (isAssertUnassert&& II->getPPKeywordID() == tok::pp_assert)<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> return Diag(AssertionPredTok, diag::err_assert_predicate_name);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> return false;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">}<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">// diagnose non-identifiers.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+//===----------------------------------------------------------------------===//<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+// Preprocessor Assertion Directive Handling.<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+//===----------------------------------------------------------------------===//<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+void Preprocessor::HandleAssertDirective(Token&DTok) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ Token AssertionPredicateTok;<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ ReadAssertionPredicate(AssertionPredicateTok, 1);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ if (<a href="http://AssertionPredicateTok.is" target="_blank">AssertionPredicateTok.is</a>(tok::eod)) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ return;<br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ AssertionInfo *AI =<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ AllocateAssertionInfo(AssertionPredicateTok.getLocation());<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ Token Tok;<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ LexUnexpandedToken(Tok);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ ReadAssertionValue(Tok, AI);<br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // add to AI<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ if (AI != 0) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">I find this interface to ReadAssertionValue to be a bit odd. How about having ReadAssertionValue lex all of the tokens it needs to, then return an allocated AssertionInfo* on success or null to indicate failure? It would simplify control flow for its callers.<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+void Preprocessor::HandleUnassertDirective(Token&DTok) {<br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+ Token AssertionPredicateTok;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ ReadAssertionPredicate(AssertionPredicateTok, 2);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // check to see if we read eod which indicates an error has occurred and<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // an appropriate diagnostic has already been generated<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (<a href="http://AssertionPredicateTok.is" target="_blank">AssertionPredicateTok.is</a>(tok::eod)) {<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ return;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ Token Tok;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ LexUnexpandedToken(Tok);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ IdentifierInfo *II = AssertionPredicateTok.getIdentifierInfo();<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // we have a valid identifier so we need to create an AssertionInfo to hold<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // the value to associate with the predicate<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ assertion_iterator iter = AssertedValues.find(II);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // we don't have an entry in the assertion table for the predicate, this is a<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // noop so escape early<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (iter == assertion_end()) {<br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+ return;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Shouldn't we still parse the rest of the #unassert to make sure it's well-formed? Or, at the very least, skip to the end of the directive?<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+void Preprocessor::ReadAssertionValue(Token&Tok, AssertionInfo *AI) {<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (Tok.isNot(tok::l_paren)) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // emit diag<br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">+ ReleaseAssertionInfo(AI);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ AI = 0;<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ return DiscardUntilEndOfDirective();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">You're missing the diagnostic here.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">+ while (true) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ LexUnexpandedToken(Tok);<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ if (<a href="http://Tok.is" target="_blank">Tok.is</a>(tok::r_paren) || <a href="http://Tok.is" target="_blank">Tok.is</a>(tok::eod)) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ break;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ AI->AddToken(Tok);<br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Could you add a comment saying that it's intentional that parentheses don't nest?It surprised me, but I see that's the documented behavior, and I don't want someone coming through later and "fixing" it.<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // check for the closing ), if we don't have it relase the AssetionInfo and<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // discard the rest of the directive<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (!Tok.is(tok::r_paren)) {<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ ReleaseAssertionInfo(AI);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ AI = 0;<br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">+ return DiscardUntilEndOfDirective();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Comment typo "relase". Also, shouldn't we emit a diagnostic here?<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // check there is nothing after the closing ), if we have anything release<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ // the Assertioninfo and idscard the rest of the directive<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ LexUnexpandedToken(Tok);<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ if (!Tok.is(tok::eod)) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ ReleaseAssertionInfo(AI);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
+ AI = 0;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ return DiscardUntilEndOfDirective();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Diagnostic here?<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ while (true) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ PP.LexUnexpandedToken(PeekTok);<br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+ if (<a href="http://PeekTok.is" target="_blank">PeekTok.is</a>(tok::r_paren) || <a href="http://PeekTok.is" target="_blank">PeekTok.is</a>(tok::eod)) {<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ break;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite">
<blockquote type="cite">+ AI->AddToken(PeekTok);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Same request for a comment about non-nesting parens.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote>
</blockquote><blockquote type="cite"><blockquote type="cite">+ // check for the closing ), if we don't have it relase the AssertionInfo<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // and discard the rest of the directive<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (!PeekTok.is(tok::r_paren)) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ PP.Diag(PredicateTok, diag::err_pp_expr_bad_token_start_expr);<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ PP.ReleaseAssertionInfo(AI);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ AI = 0;<br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">+ return true;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">"relase" typo in comment.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite">This patch is looking really good. One non-trivial piece that's still missing is that the AssertedValues map will need to be serialized to the AST file and (lazily) deserialized, so that this feature works with precompiled headers.<br>
</blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">pp_assert_default.patch looks good to me.<br></blockquote></blockquote>
<blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Thanks for working on this!<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">
<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span style="white-space:pre-wrap"> </span>- Doug<br></blockquote></blockquote><blockquote type="cite">_______________________________________________<br>
</blockquote><blockquote type="cite">cfe-commits mailing list<br></blockquote><blockquote type="cite"><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br></blockquote><blockquote type="cite">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote><span><pp_assert_defaults.patch></span><span><pp_assert_impl.patch></span><span><pp_assert_serialization.patch></span><span><boost_usuallytinyptrvector_to_basic.patch></span>_______________________________________________<br>
cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></blockquote></div><br></div><br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>