<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Argyrios,<br>
<br>
Thank you for reviewing my patches.<br>
<br>
I have replied to your comments inline below.<br>
<br>
I ran into preprocessor assertions when I was trying to build an
older version of Grub which is what prompted my implementation.<br>
<br>
Kind regards,<br>
Andrew<br>
<br>
On 07-Mar-12 03:21, Argyrios Kyrtzidis wrote:
<blockquote
cite="mid:7A927C85-3360-4858-BCFF-CBFE5C8420F4@apple.com"
type="cite">
<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 moz-do-not-send="true"
href="http://gcc.gnu.org/onlinedocs/gcc-3.1/cpp/Assertions.html">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 moz-do-not-send="true"
href="http://ideone.com">http://ideone.com</a>, gcc still
supports them on <span class="Apple-style-span" style="color:
rgb(51, 51, 51); font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">gcc-4.3.4.</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; "><br>
</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">
<div style="color: rgb(0, 0, 0);
-webkit-border-horizontal-spacing: 0px;
-webkit-border-vertical-spacing: 0px; font-size: medium; "><span
class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">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 class="Apple-style-span" style="color: rgb(51, 51,
51); font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">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
class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">and I'm worried about
the maintenance burden that this deprecated feature will
impose, e.g.</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; "><br>
</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">-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
class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">IdentifierInfo
object's size.</span></div>
</blockquote>
If anyone has suggestions on how to avoid this, I am happy to
consider reworking my patch to incorporate such an improvement.
<blockquote
cite="mid:7A927C85-3360-4858-BCFF-CBFE5C8420F4@apple.com"
type="cite">
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; "><br>
</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">-it increases the size
of the IdentifierInfo data that are stored in the serialized
AST.</span></div>
</blockquote>
I think this is unavoidable if the feature is going to be
implemented, but again suggestions on how to improve things are most
welcome.<br>
<blockquote
cite="mid:7A927C85-3360-4858-BCFF-CBFE5C8420F4@apple.com"
type="cite">
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; "><br>
</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">Although the changes
are extensive, there may be more that are needed:</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; "><br>
</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">-gcc docs say that you
can specify assertions by command-line options; I did not see
this implemented in Andrew's patches</span></div>
</blockquote>
I am aware the command-line option (-A) to assert a key-value pair
is not implemented in the patches I have submitted. I didn't write
this support yet because clang uses -A for other things and I wasn't
sure if that needed to be changed or if we were going to use a
different flag - the actual patch will be relatively small. If the
feature is accepted and guidance is provided as to what flag we
should use I am happy to write and contribute this patch
<blockquote
cite="mid:7A927C85-3360-4858-BCFF-CBFE5C8420F4@apple.com"
type="cite">
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; "><br>
</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">-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>
</blockquote>
I do not believe there is anything that needs to be validated in
terms of PCH vs command-line vs source preprocessor assertions. You
can assert multiple values for a given key quite correctly (unlike
macros) and assertions are designed to be added and removed using
all of these features and it is not wrong to do so. Further,
asserting the same key-value pair twice is not an error and so I
don't think there needs to be validation of clashes since, by
design, these clashes are permitted.
<blockquote
cite="mid:7A927C85-3360-4858-BCFF-CBFE5C8420F4@apple.com"
type="cite">
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; "><br>
</span></div>
<div><span class="Apple-style-span" style="color: rgb(51, 51, 51);
font-size: 13px; -webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px; ">-It is not clear to me
how this feature should interact with modules.</span></div>
</blockquote>
I am not sure how these would interact since the reference
implementation, GCC, does not support modules in the versions I have
tested. Doug, do you have any thoughts on this since you have worked
on modules? Since the behavior is undefined we can probably do
whatever is easiest and makes the most sense.
<blockquote
cite="mid:7A927C85-3360-4858-BCFF-CBFE5C8420F4@apple.com"
type="cite">
<div><font class="Apple-style-span" color="#333333"><span
class="Apple-style-span" style="font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px;"><br>
</span></font></div>
<div><font class="Apple-style-span" color="#333333"><span
class="Apple-style-span" style="font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px;"><br>
</span></font></div>
<div><font class="Apple-style-span" color="#333333"><span
class="Apple-style-span" style="font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px;">So, to recap, the
question is this, should we implement and maintain a gcc
extension that was deprecated since gcc 3 ?</span></font></div>
</blockquote>
I think clang should implement and maintain this feature because<br>
1) It's widely supported by existing compilers including GCC 4.6.X (<a
class="moz-txt-link-freetext"
href="http://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Preprocessor-Options.html#Preprocessor-Options">http://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Preprocessor-Options.html#Preprocessor-Options</a>),
Intel C/C++ (<a class="moz-txt-link-freetext"
href="http://denali.princeton.edu/intel_cc_docs/c_ug/index.htm">http://denali.princeton.edu/intel_cc_docs/c_ug/index.htm</a>),
and ARM's toolchain
(<a class="moz-txt-link-freetext"
href="http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CHDFDIDE.html">http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CHDFDIDE.html</a>)<br>
2) Preprocessor assertions are an optional extension defined in
System V Release 4 (SVR4) see the System V Interface manual Vol 3
page 270 (<a class="moz-txt-link-freetext"
href="http://www.sco.com/developers/devspecs/vol3_ps.ps">http://www.sco.com/developers/devspecs/vol3_ps.ps</a>)
and so should be implemented to support SVR4 systems which have
chosen to implement this feature.<br>
3) It was clearly identified as a missing feature by FIXMEs in
Chris' original commit of PPDirectives.cpp and currently the
compiler produces only cryptic errors when this feature is
encountered.<br>
4) It is not possible to work around this feature without having to
modify the source and doing so with an automated sed or awk script
is risky and difficult to integrate into some build processes. I did
consider trying to emulate assertions using macros, but I found this
wasn't possible using only the preprocessor. Asserts and macros
exist in different namespaces and so you can define and assert the
same symbol. In many cases this is not a problem because all we want
to do is test if a macro is defined, but if the value of the macro
is important then a collision in the names would cause a problem.
The assertion tests themselves would have to be found and rewritten
using a sed or awk script which is a fragile hack rather than a
proper fix and having to run this script before a compile would
complicate the build process etc. This kind of workaround also
assumes that the source can be modified prior to compilation.<br>
5) Clang has publicly stated that it will aim for GCC compatibility
where possible and these patches make compatibility with this
feature possible.
<blockquote
cite="mid:7A927C85-3360-4858-BCFF-CBFE5C8420F4@apple.com"
type="cite">
<div><font class="Apple-style-span" color="#333333"><span
class="Apple-style-span" style="font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px;"><br>
</span></font></div>
<div><font class="Apple-style-span" color="#333333"><span
class="Apple-style-span" style="font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px;">-Argyrios</span></font></div>
<div><font class="Apple-style-span" color="#333333"><span
class="Apple-style-span" style="font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px;"><br>
</span></font></div>
<div><font class="Apple-style-span" color="#333333"><span
class="Apple-style-span" style="font-size: 13px;
-webkit-border-horizontal-spacing: 2px;
-webkit-border-vertical-spacing: 2px;"><br>
</span></font></div>
<div>
<div>On Dec 14, 2011, at 9:26 PM, Andrew Craik wrote:</div>
<br class="Apple-interchange-newline">
<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
class="Apple-tab-span" style="white-space:pre"> </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
class="Apple-tab-span" style="white-space:pre"> </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 moz-do-not-send="true"
href="http://llvm.org/docs/CodingStandards.html#ll_naming">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 moz-do-not-send="true"
href="http://AssertionPredicateTok.is">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 moz-do-not-send="true"
href="http://AssertionPredicateTok.is">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 moz-do-not-send="true"
href="http://Tok.is">Tok.is</a>(tok::r_paren) || <a
moz-do-not-send="true" href="http://Tok.is">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
moz-do-not-send="true" href="http://PeekTok.is">PeekTok.is</a>(tok::r_paren)
|| <a moz-do-not-send="true" href="http://PeekTok.is">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 class="Apple-tab-span"
style="white-space:pre"> </span>- Doug<br>
</blockquote>
</blockquote>
<blockquote type="cite">_______________________________________________<br>
</blockquote>
<blockquote type="cite">cfe-commits mailing list<br>
</blockquote>
<blockquote type="cite"><a moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
</blockquote>
<blockquote type="cite"><a moz-do-not-send="true"
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">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 moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div>
</blockquote>
</div>
<br>
</blockquote>
</body>
</html>