[cfe-dev] extern "C" support

[Mike Stump]

On Jan 6, 2008, at 1:35 PM, Chris Lattner wrote:
> On Jan 2, 2008, at 2:22 PM, Mike Stump wrote:
>> On Dec 22, 2007, at 5:28 PM, Chris Lattner wrote:
>> I did that merely because we're calling them ASTs and because we  
>> form ParenExprs.  If we didn't want ASTs, we could get rid of  
>> ParenExprs?
>
> I don't understand what you're saying here.  ASTs can take many forms.

Sorry, the eyes read AST, but the brain was thinking parse trees, due  
to ParenExprs.

> You seem to be using an overly constrained definition of what an AST  
> is :).

:-)  Yeah, I agree, we don't need to preserve the parse tree in the AST!

> In this specific case, I'd be fine with -ast-print always emitting  
> {}'s for example.

Done.

> Please add FIXME markers to everything you *know* is incomplete or  
> incorrect.

Ok.

> Many of the changes to AST/Decl.cpp and the change to Type.h are  
> unrelated to this patch.  It would be good to split them out as they  
> aren't controversial at all :)

I sent the Type.h separately.  I'm holding the other hostage.  :-)   
Oops, it went missing in the Objc ObjC rewrite, oh well...

> +void clang::CodeGen::CodeGenLinkageSpec(CodeGenModule *B,  
> LinkageSpecDecl *LS) {
> +
> +}
>
> Please have this emit an unsupported warning or something (see  
> CodeGenModule::WarnUnsupported) so that we know when this is  
> happening.

Gosh, none of the existing ones could be used, and I didn't want to  
learn how to do all that...  [ pause ]  Oh well.  :-)  Done.

> Also, please add a FIXME.

Done.

> +/// LinkageSpecDecl - This represents a linkage specification.
> +class LinkageSpecDecl : public Decl {
>
> As I requested previously, please add an example.

> "Language language" is strange.  I'd suggest renaming the enum to  
> LanguageIDs or something.

Done.

> +  bool inside_braces;
>
> If you want to keep this

Gone.

> How about getDecls() ?

Done.

> Please stay in 80 columns and use mixed caps instead of _'s.

Done.

> Ok, but please format with the llvm style (see surrounding code)  
> which has the &&'s on the previous line, no space before the (), and  
> either drop the {} or make them K&R style.

Done.

>  Likewise in the implementation of ParseLinkage.

Done.

> +// C++ 7.5p2
> +Parser::DeclTy *Parser::ParseLinkage(unsigned Context) {
> +  std::string lang = PP.getSpelling(Tok);
>
> Please add an assertion that the Tok.is(stringliteral), and a  
> comment above the method, like all other parser methods have.

Done.

> Also, this is the expensive form of getSpelling, [ ... ] Instead,  
> please use something like this:

Done.

> In addition to the formatting issues above, please rearrange this to  
> make the small case come first.

Done.

> Also, RBrace is dead, so you can just remove its declaration.

Done....

> Better yet, you could pass *it* into ActOnLinkageSpec instead of  
> 'inside_brace'.

Sure, done.

> +  return Actions.ActOnLinkageSpec(Loc, lang.c_str(), saw_braces, D);
>
> Can D be null here?

Sure.

> If so, you should check for it and return an error instead of  
> building an invalid AST node.

Done.

> +Sema::DeclTy* Sema::ActOnLinkageSpec(SourceLocation Loc,
> +				     const char *Lang,
> +				     bool inside_brace,
> +				     DeclTy *D) {
>
> This looks good, but please clean up the formatting above and add a  
> fixme if there are semantic checks missing :)

Well, almost all of them are missing...  :-)  Added a FIXME.

Let's try this version:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: externc-2.patch
Type: application/octet-stream
Size: 13418 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080108/c6b5be29/attachment.obj>
-------------- next part --------------