<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
*bump* Was just wondering if anyone had had a look at this patch as
I don't have commit access.<br>
<br>
Cheers,<br>
Andrew<br>
<br>
On 15-Dec-11 15:26, Andrew Craik wrote:
<blockquote cite="mid:4EE984EE.3070905@oracle.com" type="cite">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>
<br>
Thanks for the detailed feedback on the code style and control
flow; all
<br>
your comments seem very reasonable and I will go and rework the
patch. I
<br>
will see if I can add another patch to do the
<br>
serialization/de-serialization.
<br>
<br>
Andrew
<br>
<br>
On 04-Dec-11 06:32, Douglas Gregor wrote:
<br>
<blockquote type="cite">On Dec 1, 2011, at 11:46 PM, Andrew
Craik wrote:
<br>
<br>
<blockquote type="cite">Hello,
<br>
<br>
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>
Please find attached two patches:
<br>
pp_assert_impl.patch - this adds handling of assertions to
the pre-processor
<br>
pp_assert_default.patch - this adds default cpu and system
assertions to Targets.cpp
<br>
<br>
I look forward to hearing from the community and hopefully
getting these patches integrated.
<br>
</blockquote>
Thanks. A bunch of comments below, but generally things are
look very good!
<br>
<br>
Starting with pp_assert_impl.patch:
<br>
<br>
Index: clang/include/clang/Lex/Preprocessor.h
<br>
===================================================================
<br>
--- clang.orig/include/clang/Lex/Preprocessor.h 2011-11-30
11:39:19.000000000 +1000
<br>
+++ clang/include/clang/Lex/Preprocessor.h 2011-12-02
12:23:53.246940900 +1000
<br>
@@ -15,6 +15,7 @@
<br>
#define LLVM_CLANG_LEX_PREPROCESSOR_H
<br>
<br>
#include "clang/Lex/MacroInfo.h"
<br>
+#include "clang/Lex/AssertionInfo.h"
<br>
#include "clang/Lex/Lexer.h"
<br>
#include "clang/Lex/PTHLexer.h"
<br>
#include "clang/Lex/PPCallbacks.h"
<br>
@@ -240,6 +241,10 @@
<br>
/// to the actual definition of the macro.
<br>
llvm::DenseMap<IdentifierInfo*, MacroInfo*>
Macros;
<br>
<br>
+ /// Assertions - For each IdentifierInfo that has appeared
as an asserted
<br>
+ /// predicate, we store the values for which it was
asserted
<br>
+ llvm::DenseMap<IdentifierInfo*,
std::vector<AssertionInfo*> > AssertedValues;
<br>
<br>
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>
<br>
@@ -440,6 +459,30 @@
<br>
macro_iterator macro_begin(bool IncludeExternalMacros =
true) const;
<br>
macro_iterator macro_end(bool IncludeExternalMacros =
true) const;
<br>
<br>
+ /// AddAssertionValue - assert the specified identifier to
the
<br>
+ /// set of asserted values for the given assertion key
<br>
+ void AddAssertionValue(IdentifierInfo *II, AssertionInfo
*AI);
<br>
+
<br>
+ /// IsAsserted - test if the specified identifier has been
asserted as a
<br>
+ /// value of the given assertion key returns true iff it
has
<br>
+ bool IsAsserted(IdentifierInfo *II, AssertionInfo *AI);
<br>
+
<br>
+ /// RemoveAssertionValue - remove the specified identifier
from the set of
<br>
+ /// asserted values for the given assertion key
<br>
+ void RemoveAssertionValue(IdentifierInfo *II, AssertionInfo
*AI);
<br>
+
<br>
+ /// RemoveAssertion - remove the specified assertion and
all of its asserted
<br>
+ /// values
<br>
+ void RemoveAssertion(IdentifierInfo *II);
<br>
<br>
A quick request here… the LLVM coding standards clarified the
naming of functions a few months ago
(<a class="moz-txt-link-freetext" 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>
The same comment applies throughout the patch, because we want
new code to follow this convention and existing code to
eventually migrate over.
<br>
<br>
<br>
+ void Destory() {
<br>
+ ValueTokens.clear();
<br>
+ this->~AssertionInfo();
<br>
+ }
<br>
<br>
Typo "Destory", propagated throughout.
<br>
<br>
+/// ReadAssertionPredicate - Lex and validate an assertion
predicate, which
<br>
+/// occurs after an #assert or #unassert. This sets the token
kind to eod and
<br>
+/// discards the rest of the line if the assert is invalid.
<br>
+/// isAssertUnassert is 1 if this is due to an #assert, 2 if
#unassert
<br>
+/// directive, 0 if it is something else
<br>
+void
Preprocessor::ReadAssertionPredicate(Token&AssertionPredTok,
<br>
+ char isAssertUnassert) {
<br>
<br>
Please use an enumeration type rather than 0/1/2 here.
<br>
<br>
Also, why not indicate failure by returning 'true' and success
by returning 'false', as we do elsewhere in Clang?
<br>
<br>
+ IdentifierInfo *II = AssertionPredTok.getIdentifierInfo();
<br>
+ if (II == 0) {
<br>
+ bool Invalid = false;
<br>
+ std::string Spelling =
getSpelling(AssertionPredTok,&Invalid);
<br>
+ if (Invalid) {
<br>
+ return;
<br>
+ }
<br>
+
<br>
+ const IdentifierInfo&Info =
Identifiers.get(Spelling);
<br>
+ if (Info.isCPlusPlusOperatorKeyword()) {
<br>
+ Diag(AssertionPredTok,
diag::err_pp_operator_used_as_assert_predicate)
<br>
+<< Spelling;
<br>
+ } else {
<br>
+ Diag(AssertionPredTok,
diag::err_pp_predicate_not_identifier);
<br>
+ }
<br>
+ // Fall through on error.
<br>
+ } else if (isAssertUnassert&&
II->getPPKeywordID() == tok::pp_assert) {
<br>
+ // Error if using assert as a predicate since using it
would require
<br>
+ // #assert
<br>
+ Diag(AssertionPredTok, diag::err_assert_predicate_name);
<br>
+ } else {
<br>
+ // Okay, we got a good identifier node. Return it.
<br>
+ return;
<br>
+ }
<br>
<br>
If you did switch the return value to 'bool', this code could
be de-nested and clarified a bit:
<br>
<br>
if (IdentifierInfo *II = AssertionPredTok.getIdentifierInfo())
{
<br>
if (isAssertUnassert&& II->getPPKeywordID()
== tok::pp_assert)
<br>
return Diag(AssertionPredTok,
diag::err_assert_predicate_name);
<br>
<br>
return false;
<br>
}
<br>
<br>
// diagnose non-identifiers.
<br>
<br>
+//===----------------------------------------------------------------------===//
<br>
+// Preprocessor Assertion Directive Handling.
<br>
+//===----------------------------------------------------------------------===//
<br>
+void Preprocessor::HandleAssertDirective(Token&DTok) {
<br>
+ Token AssertionPredicateTok;
<br>
+ ReadAssertionPredicate(AssertionPredicateTok, 1);
<br>
+
<br>
+ if (AssertionPredicateTok.is(tok::eod)) {
<br>
+ return;
<br>
+ }
<br>
+
<br>
+ AssertionInfo *AI =
<br>
+
AllocateAssertionInfo(AssertionPredicateTok.getLocation());
<br>
+ Token Tok;
<br>
+ LexUnexpandedToken(Tok);
<br>
+ ReadAssertionValue(Tok, AI);
<br>
+
<br>
+ // add to AI
<br>
+ if (AI != 0) {
<br>
<br>
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>
<br>
+void Preprocessor::HandleUnassertDirective(Token&DTok) {
<br>
+ Token AssertionPredicateTok;
<br>
+ ReadAssertionPredicate(AssertionPredicateTok, 2);
<br>
+
<br>
+ // check to see if we read eod which indicates an error has
occurred and
<br>
+ // an appropriate diagnostic has already been generated
<br>
+ if (AssertionPredicateTok.is(tok::eod)) {
<br>
+ return;
<br>
+ }
<br>
+
<br>
+ Token Tok;
<br>
+ LexUnexpandedToken(Tok);
<br>
+
<br>
+ IdentifierInfo *II =
AssertionPredicateTok.getIdentifierInfo();
<br>
+
<br>
+ // we have a valid identifier so we need to create an
AssertionInfo to hold
<br>
+ // the value to associate with the predicate
<br>
+ assertion_iterator iter = AssertedValues.find(II);
<br>
+
<br>
+ // we don't have an entry in the assertion table for the
predicate, this is a
<br>
+ // noop so escape early
<br>
+ if (iter == assertion_end()) {
<br>
+ return;
<br>
+ }
<br>
<br>
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>
<br>
+void Preprocessor::ReadAssertionValue(Token&Tok,
AssertionInfo *AI) {
<br>
+ if (Tok.isNot(tok::l_paren)) {
<br>
+ // emit diag
<br>
+ ReleaseAssertionInfo(AI);
<br>
+ AI = 0;
<br>
+ return DiscardUntilEndOfDirective();
<br>
+ }
<br>
<br>
You're missing the diagnostic here.
<br>
<br>
+ while (true) {
<br>
+ LexUnexpandedToken(Tok);
<br>
+ if (Tok.is(tok::r_paren) || Tok.is(tok::eod)) {
<br>
+ break;
<br>
+ }
<br>
+ AI->AddToken(Tok);
<br>
+ }
<br>
<br>
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>
<br>
+ // check for the closing ), if we don't have it relase the
AssetionInfo and
<br>
+ // discard the rest of the directive
<br>
+ if (!Tok.is(tok::r_paren)) {
<br>
+ ReleaseAssertionInfo(AI);
<br>
+ AI = 0;
<br>
+ return DiscardUntilEndOfDirective();
<br>
+ }
<br>
<br>
Comment typo "relase". Also, shouldn't we emit a diagnostic
here?
<br>
<br>
+ // check there is nothing after the closing ), if we have
anything release
<br>
+ // the Assertioninfo and idscard the rest of the directive
<br>
+ LexUnexpandedToken(Tok);
<br>
+ if (!Tok.is(tok::eod)) {
<br>
+ ReleaseAssertionInfo(AI);
<br>
+ AI = 0;
<br>
+ return DiscardUntilEndOfDirective();
<br>
+ }
<br>
<br>
Diagnostic here?
<br>
<br>
+ while (true) {
<br>
+ PP.LexUnexpandedToken(PeekTok);
<br>
+ if (PeekTok.is(tok::r_paren) || PeekTok.is(tok::eod)) {
<br>
+ break;
<br>
+ }
<br>
+ AI->AddToken(PeekTok);
<br>
+ }
<br>
<br>
Same request for a comment about non-nesting parens.
<br>
<br>
+ // check for the closing ), if we don't have it relase
the AssertionInfo
<br>
+ // and discard the rest of the directive
<br>
+ if (!PeekTok.is(tok::r_paren)) {
<br>
+ PP.Diag(PredicateTok,
diag::err_pp_expr_bad_token_start_expr);
<br>
+ PP.ReleaseAssertionInfo(AI);
<br>
+ AI = 0;
<br>
+ return true;
<br>
+ }
<br>
<br>
"relase" typo in comment.
<br>
<br>
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>
<br>
pp_assert_default.patch looks good to me.
<br>
<br>
Thanks for working on this!
<br>
<br>
- Doug
<br>
</blockquote>
_______________________________________________
<br>
cfe-commits mailing list
<br>
<a class="moz-txt-link-abbreviated" 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>
</blockquote>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
cfe-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>
<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>
</pre>
</blockquote>
</body>
</html>