[cfe-commits] [PATCH] GCC Preprocessor Assertions revision 2
Andrew Craik
andrew.craik at oracle.com
Wed Dec 14 21:26:06 PST 2011
Hi Doug,
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:
1) boost_usuallytinyptrvector_to_basic.patch - moves the
UsuallyPtrVector type to Basic, per your previous, and adds an erase
method to it
2) pp_assert_impl.patch - reworked version of the original patch to
implement handling of asserts in the pre-processor
3) pp_assert_defaults.patch - same as previous post adding the default
asserts for systems and cpus
4) pp_assert_serialization.patch - adds serialization support for
assertions and all the required callbacks etc
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.
I look forward to hearing from you.
Andrew
On 04-Dec-11 08:33, Andrew Craik wrote:
> Hi Doug,
>
> Thanks for the detailed feedback on the code style and control flow; all
> your comments seem very reasonable and I will go and rework the patch. I
> will see if I can add another patch to do the
> serialization/de-serialization.
>
> Andrew
>
> On 04-Dec-11 06:32, Douglas Gregor wrote:
>> On Dec 1, 2011, at 11:46 PM, Andrew Craik wrote:
>>
>>> Hello,
>>>
>>> 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.
>>> Please find attached two patches:
>>> pp_assert_impl.patch - this adds handling of assertions to the pre-processor
>>> pp_assert_default.patch - this adds default cpu and system assertions to Targets.cpp
>>>
>>> I look forward to hearing from the community and hopefully getting these patches integrated.
>> Thanks. A bunch of comments below, but generally things are look very good!
>>
>> Starting with pp_assert_impl.patch:
>>
>> Index: clang/include/clang/Lex/Preprocessor.h
>> ===================================================================
>> --- clang.orig/include/clang/Lex/Preprocessor.h 2011-11-30 11:39:19.000000000 +1000
>> +++ clang/include/clang/Lex/Preprocessor.h 2011-12-02 12:23:53.246940900 +1000
>> @@ -15,6 +15,7 @@
>> #define LLVM_CLANG_LEX_PREPROCESSOR_H
>>
>> #include "clang/Lex/MacroInfo.h"
>> +#include "clang/Lex/AssertionInfo.h"
>> #include "clang/Lex/Lexer.h"
>> #include "clang/Lex/PTHLexer.h"
>> #include "clang/Lex/PPCallbacks.h"
>> @@ -240,6 +241,10 @@
>> /// to the actual definition of the macro.
>> llvm::DenseMap<IdentifierInfo*, MacroInfo*> Macros;
>>
>> + /// Assertions - For each IdentifierInfo that has appeared as an asserted
>> + /// predicate, we store the values for which it was asserted
>> + llvm::DenseMap<IdentifierInfo*, std::vector<AssertionInfo*> > AssertedValues;
>>
>> 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).
>>
>> @@ -440,6 +459,30 @@
>> macro_iterator macro_begin(bool IncludeExternalMacros = true) const;
>> macro_iterator macro_end(bool IncludeExternalMacros = true) const;
>>
>> + /// AddAssertionValue - assert the specified identifier to the
>> + /// set of asserted values for the given assertion key
>> + void AddAssertionValue(IdentifierInfo *II, AssertionInfo *AI);
>> +
>> + /// IsAsserted - test if the specified identifier has been asserted as a
>> + /// value of the given assertion key returns true iff it has
>> + bool IsAsserted(IdentifierInfo *II, AssertionInfo *AI);
>> +
>> + /// RemoveAssertionValue - remove the specified identifier from the set of
>> + /// asserted values for the given assertion key
>> + void RemoveAssertionValue(IdentifierInfo *II, AssertionInfo *AI);
>> +
>> + /// RemoveAssertion - remove the specified assertion and all of its asserted
>> + /// values
>> + void RemoveAssertion(IdentifierInfo *II);
>>
>> A quick request here… the LLVM coding standards clarified the naming of functions a few months ago (http://llvm.org/docs/CodingStandards.html#ll_naming), so please name these addAssertionValue, isAsserted, and so on.
>> The same comment applies throughout the patch, because we want new code to follow this convention and existing code to eventually migrate over.
>>
>>
>> + void Destory() {
>> + ValueTokens.clear();
>> + this->~AssertionInfo();
>> + }
>>
>> Typo "Destory", propagated throughout.
>>
>> +/// ReadAssertionPredicate - Lex and validate an assertion predicate, which
>> +/// occurs after an #assert or #unassert. This sets the token kind to eod and
>> +/// discards the rest of the line if the assert is invalid.
>> +/// isAssertUnassert is 1 if this is due to an #assert, 2 if #unassert
>> +/// directive, 0 if it is something else
>> +void Preprocessor::ReadAssertionPredicate(Token&AssertionPredTok,
>> + char isAssertUnassert) {
>>
>> Please use an enumeration type rather than 0/1/2 here.
>>
>> Also, why not indicate failure by returning 'true' and success by returning 'false', as we do elsewhere in Clang?
>>
>> + IdentifierInfo *II = AssertionPredTok.getIdentifierInfo();
>> + if (II == 0) {
>> + bool Invalid = false;
>> + std::string Spelling = getSpelling(AssertionPredTok,&Invalid);
>> + if (Invalid) {
>> + return;
>> + }
>> +
>> + const IdentifierInfo&Info = Identifiers.get(Spelling);
>> + if (Info.isCPlusPlusOperatorKeyword()) {
>> + Diag(AssertionPredTok, diag::err_pp_operator_used_as_assert_predicate)
>> +<< Spelling;
>> + } else {
>> + Diag(AssertionPredTok, diag::err_pp_predicate_not_identifier);
>> + }
>> + // Fall through on error.
>> + } else if (isAssertUnassert&& II->getPPKeywordID() == tok::pp_assert) {
>> + // Error if using assert as a predicate since using it would require
>> + // #assert
>> + Diag(AssertionPredTok, diag::err_assert_predicate_name);
>> + } else {
>> + // Okay, we got a good identifier node. Return it.
>> + return;
>> + }
>>
>> If you did switch the return value to 'bool', this code could be de-nested and clarified a bit:
>>
>> if (IdentifierInfo *II = AssertionPredTok.getIdentifierInfo()) {
>> if (isAssertUnassert&& II->getPPKeywordID() == tok::pp_assert)
>> return Diag(AssertionPredTok, diag::err_assert_predicate_name);
>>
>> return false;
>> }
>>
>> // diagnose non-identifiers.
>>
>> +//===----------------------------------------------------------------------===//
>> +// Preprocessor Assertion Directive Handling.
>> +//===----------------------------------------------------------------------===//
>> +void Preprocessor::HandleAssertDirective(Token&DTok) {
>> + Token AssertionPredicateTok;
>> + ReadAssertionPredicate(AssertionPredicateTok, 1);
>> +
>> + if (AssertionPredicateTok.is(tok::eod)) {
>> + return;
>> + }
>> +
>> + AssertionInfo *AI =
>> + AllocateAssertionInfo(AssertionPredicateTok.getLocation());
>> + Token Tok;
>> + LexUnexpandedToken(Tok);
>> + ReadAssertionValue(Tok, AI);
>> +
>> + // add to AI
>> + if (AI != 0) {
>>
>> 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.
>>
>> +void Preprocessor::HandleUnassertDirective(Token&DTok) {
>> + Token AssertionPredicateTok;
>> + ReadAssertionPredicate(AssertionPredicateTok, 2);
>> +
>> + // check to see if we read eod which indicates an error has occurred and
>> + // an appropriate diagnostic has already been generated
>> + if (AssertionPredicateTok.is(tok::eod)) {
>> + return;
>> + }
>> +
>> + Token Tok;
>> + LexUnexpandedToken(Tok);
>> +
>> + IdentifierInfo *II = AssertionPredicateTok.getIdentifierInfo();
>> +
>> + // we have a valid identifier so we need to create an AssertionInfo to hold
>> + // the value to associate with the predicate
>> + assertion_iterator iter = AssertedValues.find(II);
>> +
>> + // we don't have an entry in the assertion table for the predicate, this is a
>> + // noop so escape early
>> + if (iter == assertion_end()) {
>> + return;
>> + }
>>
>> 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?
>>
>> +void Preprocessor::ReadAssertionValue(Token&Tok, AssertionInfo *AI) {
>> + if (Tok.isNot(tok::l_paren)) {
>> + // emit diag
>> + ReleaseAssertionInfo(AI);
>> + AI = 0;
>> + return DiscardUntilEndOfDirective();
>> + }
>>
>> You're missing the diagnostic here.
>>
>> + while (true) {
>> + LexUnexpandedToken(Tok);
>> + if (Tok.is(tok::r_paren) || Tok.is(tok::eod)) {
>> + break;
>> + }
>> + AI->AddToken(Tok);
>> + }
>>
>> 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.
>>
>> + // check for the closing ), if we don't have it relase the AssetionInfo and
>> + // discard the rest of the directive
>> + if (!Tok.is(tok::r_paren)) {
>> + ReleaseAssertionInfo(AI);
>> + AI = 0;
>> + return DiscardUntilEndOfDirective();
>> + }
>>
>> Comment typo "relase". Also, shouldn't we emit a diagnostic here?
>>
>> + // check there is nothing after the closing ), if we have anything release
>> + // the Assertioninfo and idscard the rest of the directive
>> + LexUnexpandedToken(Tok);
>> + if (!Tok.is(tok::eod)) {
>> + ReleaseAssertionInfo(AI);
>> + AI = 0;
>> + return DiscardUntilEndOfDirective();
>> + }
>>
>> Diagnostic here?
>>
>> + while (true) {
>> + PP.LexUnexpandedToken(PeekTok);
>> + if (PeekTok.is(tok::r_paren) || PeekTok.is(tok::eod)) {
>> + break;
>> + }
>> + AI->AddToken(PeekTok);
>> + }
>>
>> Same request for a comment about non-nesting parens.
>>
>> + // check for the closing ), if we don't have it relase the AssertionInfo
>> + // and discard the rest of the directive
>> + if (!PeekTok.is(tok::r_paren)) {
>> + PP.Diag(PredicateTok, diag::err_pp_expr_bad_token_start_expr);
>> + PP.ReleaseAssertionInfo(AI);
>> + AI = 0;
>> + return true;
>> + }
>>
>> "relase" typo in comment.
>>
>> 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.
>>
>> pp_assert_default.patch looks good to me.
>>
>> Thanks for working on this!
>>
>> - Doug
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pp_assert_defaults.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111215/b3204af6/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pp_assert_impl.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111215/b3204af6/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pp_assert_serialization.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111215/b3204af6/attachment-0002.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: boost_usuallytinyptrvector_to_basic.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111215/b3204af6/attachment-0003.ksh>
More information about the cfe-commits
mailing list