[cfe-commits] Declarator::getSourceRange()

[Chris Lattner]

On Feb 8, 2009, at 1:52 PM, Sebastian Redl wrote:

> Sebastian Redl wrote:
>> Hi,
>>
>> This patch implements Declarator::getSourceRange(). No regressions  
>> here,
>> but since this is such an important part of parsing, and also  
>> because I
>> don't really know how to write tests for this, I'd like people to  
>> have a
>> look at it before I commit.
>> The validation of new expressions uses the call a bit (some changes  
>> for
>> this are also in the patch), but that's all the testing that's  
>> really done.
>>
>
> Uhh ... actual patch.


Thanks for tackling this!  The patch is looking good but here are some  
minor points:

+++ lib/AST/Type.cpp	(working copy)
@@ -96,7 +96,7 @@
  }

  bool Type::isObjectType() const {
-  if (isa<FunctionType>(CanonicalType))
+  if (isa<FunctionType>(CanonicalType) ||  
isa<ReferenceType>(CanonicalType))
      return false;
    if (const ASQualType *AS = dyn_cast<ASQualType>(CanonicalType))
      return AS->getBaseType()->isObjectType();

This doesn't seem related, please commit separately.


-Parser::OwningExprResult Parser::ParseSimpleAsm() {
+Parser::OwningExprResult Parser::ParseSimpleAsm(SourceLocation  
*EndLoc) {

Please add a comment about what EndLoc is.  Also, why not pass by  
reference?

+    if (Tok.is(tok::kw___attribute)) {
+      SourceLocation Loc;
+      D.AddAttributes(ParseAttributes(&Loc));
+      D.SetRangeEnd(Loc);

Please change this to:

D.AddAttributes(ParseAttributes(&Loc), Loc);

And have AddAttributes update SetRangeEnd, this makes it so we can't  
forget to update the endloc when adding attributes.

@@ -1586,6 +1604,10 @@
        SourceLocation Loc = ConsumeToken();
        DeclSpec DS;
        ParseTypeQualifierListOpt(DS);
+      if (DS.getSourceRange().getEnd().isInvalid())
+        D.SetRangeEnd(Loc);
+      else
+        D.SetRangeEnd(DS.getSourceRange().getEnd());

This is somewhat awkward.  How about adding a  
Declarator::SetDeclSpecRange() method or something?  This happens in a  
bunch of other places too.

             
D.setConstructor(Actions.getTypeName(*Tok.getIdentifierInfo(),
                                                 Tok.getLocation(),  
CurScope),
                             Tok.getLocation());
+          D.SetRangeEnd(Tok.getLocation());

Can setConstructor update the range itself?



+        if (OverloadedOperatorKind Op =  
TryParseOperatorFunctionId(&EndLoc)) {

Why pass EndLoc by pointer instead of byref?


+  // FIXME: Since the return type isn't actually parsed, need some fake
+  // location info for the declarator.
+  ParamInfo.SetSourceRange(SourceRange(Tok.getLocation(),  
Tok.getLocation()));

Does it make sense to just leave it as 'invalid'?

      ParamInfo.AddTypeInfo(DeclaratorChunk::getFunction(true, false,
                                                         0, 0, 0,  
CaretLoc,
                                                         ParamInfo));
+    ParamInfo.SetRangeEnd(CaretLoc);


AddTypeInfo should take a loc to update the end of the range with.   
Ideally, the code should almost never call SetRangeEnd directly.

-Chris