[cfe-commits] Declarator::getSourceRange()

[Sebastian Redl]

On Sun, 8 Feb 2009 23:50:36 -0800, Chris Lattner <clattner at apple.com>
wrote:
> On Feb 8, 2009, at 1:52 PM, Sebastian Redl wrote:
> 
>>
>> 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.
> 

It's kind of part of the new expression work I did to test the thing. But
OK.

> 
> -Parser::OwningExprResult Parser::ParseSimpleAsm() {
> +Parser::OwningExprResult Parser::ParseSimpleAsm(SourceLocation  
> *EndLoc) {
> 
> Please add a comment about what EndLoc is.  Also, why not pass by  
> reference?
> 

Because I'm pretty sure that not all consumers of ParseSimpleAsm care about
the end location. I'll have to do a search to make sure.

> +    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.
> 

I'm 95% sure this is unspecified behavior. The compiler *could* take the
value of Loc for the second attribute before calling ParseAttributes(),
thus losing the update.
But I can change it to

AttributeList *Attrs = ParseAttributes(&Loc);
D.AddAttributes(Attrs, Loc);

> @@ -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.
> 

OK. I added this at the very end, as a bugfix, and was somewhat tired.

>              
> D.setConstructor(Actions.getTypeName(*Tok.getIdentifierInfo(),
>                                                  Tok.getLocation(),  
> CurScope),
>                              Tok.getLocation());
> +          D.SetRangeEnd(Tok.getLocation());
> 
> Can setConstructor update the range itself?
> 

Yes. I'll probably have to modify setDestructor and so on for consistency,
too.

> 
> 
> +        if (OverloadedOperatorKind Op =  
> TryParseOperatorFunctionId(&EndLoc)) {
> 
> Why pass EndLoc by pointer instead of byref?
> 

The other call site of the function doesn't care about the end location.

> 
> +  // 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'?

No, this leads to extremely subtle bugs in error reporting, if something
then decides to actually use the source range. The location info isn't as
fake as the comment makes it seem.

> 
>       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.
> 

Got it.

Sebastian