<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
HI Argyrios,<br>
<br>
Comments below in response to your questions and statements.<br>
<br>
Regards,<br>
Andrew<br>
<br>
On 08-Mar-12 06:18, Argyrios Kyrtzidis wrote:
<blockquote
cite="mid:7A5E88C3-354F-4AEB-B1F9-05278C38687D@apple.com"
type="cite">
<div>Hi Andrew, I'm going to comment a bit out-of-order.</div>
<div><br>
</div>
<div>
<blockquote type="cite">
<div bgcolor="#FFFFFF" text="#000000">
<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; ">-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.</div>
</blockquote>
<br>
</div>
<div>I had in mind checking whether there are differences in the
assertions in command-line and predefined ones, (missing
assertions, newly introduced), causing differences in PCH
inclusion, like we do with macros, but I noticed that gcc does
not care for assertion differences in command-line (when
including the PCH) and even if the predefined ones are
different, e.g including a PCH with</div>
<div><br>
</div>
<div>#assert cpu(i386)</div>
<div><br>
</div>
<div>when you are on x86_64, the PCH will probably be rejected by
PCH validation anyway, so we probably don't need to enhance
validation to include assertions.</div>
</blockquote>
That agrees with what I was thinking.
<blockquote
cite="mid:7A5E88C3-354F-4AEB-B1F9-05278C38687D@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div bgcolor="#FFFFFF" text="#000000">
<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; ">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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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.</div>
</blockquote>
<br>
</div>
<div>"aim for GCC compatibility" does not mean "implement the
exact behavior of gcc unconditionally".</div>
<div>There are gcc extensions that we don't have interest in
implementing, like nested functions, and when the standard
disagrees with gcc we side with the standard, see <a
moz-do-not-send="true"
href="http://clang.llvm.org/compatibility.html">http://clang.llvm.org/compatibility.html</a>.</div>
</blockquote>
I am fully aware that Clang has chosen not to implement some gcc
quirks and extensions and I appreciate that Clang is not going to
blindly implement features just because gcc does. I was not trying
to argue that Clang should do it just because gcc does it, but I was
trying to say that because GCC and other compilers implement the
feature, there is a reason Clang may want to do so as well and that
doing so would increase compatibility.
<blockquote
cite="mid:7A5E88C3-354F-4AEB-B1F9-05278C38687D@apple.com"
type="cite">
<div><br>
</div>
<div>It does not help the case for this feature that gcc itself is
aggressively discouraging people from using it, e.g. this is
from gcc 4.6.2:</div>
<div><br>
</div>
<div>
<div>$ cat t.c</div>
<div>#assert a(b)</div>
<div>#if #a(b)</div>
<div>#endif</div>
</div>
<div><br>
</div>
<div>
<div>$ gcc -fsyntax-only t.c</div>
<div>t.c:1:2: warning: #assert is a deprecated GCC extension
[-Wdeprecated]</div>
<div>t.c:2:5: warning: assertions are a deprecated extension
[-Wdeprecated]</div>
</div>
<div><br>
</div>
<div>I find the prospect that we would implement a new feature and
then put it under a "deprecated" warning, at least unfortunate.</div>
</blockquote>
While the feature is deprecated, it has been deprecated for a long
time without being removed or disabled. The deprecation has
continued across a major version and multiple minor versions, so
while it may be slated for removal eventually, the fact that it has
continued to exist for such a long time, I feel, says something
about the feature especially in light of its inclusion in the SVR4
standard and implementation in other compilers from other vendors.
<blockquote
cite="mid:7A5E88C3-354F-4AEB-B1F9-05278C38687D@apple.com"
type="cite">
<div><br>
</div>
<div>Since we in clang would discourage people from using the
feature as well, we would implement it mainly to support legacy
codebases or projects that, for some reason, still depend on a
deprecated (and not widely used) gcc extension.</div>
<div>Under the this frame of discussion:</div>
<div><br>
</div>
<div>
<blockquote type="cite">I ran into preprocessor assertions when
I was trying to build an older version of Grub which is what
prompted my implementation.</blockquote>
<br>
</div>
<div>Could you elaborate more, is this legacy codebase that nobody
wants to touch or move to a newer version ?</div>
<div>-Is the feature widely used in your company's codebase or not
?</div>
<div>-Is it only a minority of projects that you care about that
depend on it ?</div>
<div>-Could these just keep using gcc for building ?</div>
</blockquote>
Here at Oracle we are using clang to compile C/C++ source to LLVM
bitcode as part of the Parfait project (<a
class="moz-txt-link-freetext"
href="http://labs.oracle.com/projects/parfait">http://labs.oracle.com/projects/parfait</a>)
which is a highly scalable bug checking tool with a very low false
positive rate. We have migrated from llvm-gcc to clang as our
compiler for a number of reasons including the EOL announcement for
llvm-gcc.<br>
<br>
Using a standard gcc is not an option because we have to produce
bitcode. We have considered dragonegg, but it does not support all
the target platforms we support. We have encountered this feature
in codebases that are actively used in the company, these are
codebases undergoing active development and that are not in a
maintenance mode, and these products are being sold to and used by
our customers. As a result, we feel that support for this feature in
mainline Clang is needed. Unfortunately, I cannot make any more
specific comments about the content of Oracle's codebases or future
development plans. <br>
<blockquote
cite="mid:7A5E88C3-354F-4AEB-B1F9-05278C38687D@apple.com"
type="cite">
<div><br>
</div>
<div>-Argyrios</div>
<div><br>
</div>
<br>
<div>
<div>On Mar 6, 2012, at 9:27 PM, Andrew Craik wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div 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="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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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>
</div>
</blockquote>
</div>
<br>
</blockquote>
</body>
</html>