[PATCH] Improve __interface support

Clang Frontend
1

This is a rework of the previous patch I posted, this time with
__interface as a class-key rather than struct with an attribute.

All tests are currently passing, and I modified the Microsoft extension
test to account for some new capabilities.

I do not have commit access, so if this patch is acceptable I will need
someone else to commit it. If changes are required first, I am as before
happy to make them, and to later support this addition.

__interface.patch (40.8 KB)

2

Hi, nice patch.

NewFD->setVirtualAsWritten(true);

Do we need to set this on the method? According to the docs, this should only be set if virtual was explicitly written in the code.

  • if (!isDefinition || (NewTag != TTK_Class && NewTag != TTK_Struct))
  • if (OldTag == NewTag)
  • return true;
  • if (!isDefinition || !isClassCompatTagKind(NewTag) || OldTag == NewTag)
  • return true;

The behaviour of the code is changed here, though I’m not sure if that has any impact on correctness.

  • default: assert(“Invalid tag kind for field padding diagnostic!”);

You used this assert message on a couple diagnostic index helper functions which is incorrect for some. Seems to be a simple copy paste mismatch.

  • case TTK_Interface:
    Out << ‘U’;

Did you confirm that cl mangles interfaces as structs?

  • case TTK_Interface:
  • case TTK_Struct: return CXCursor_StructDecl;

I think we should add a new cursor in the C API, and not re-use CXCursor_StructDecl for interfaces.

3

   Hi, nice patch.

   NewFD->setVirtualAsWritten(true);

   Do we need to set this on the method? According to the docs, this should
   only be set if virtual was explicitly written in the code.

I don't know how else to set it. __interface implies all member
functions defined within are pure virtual, so one could say "as written"
fits the user writing "__interface" to get virtual functions. If there's
a better way to set virtual, I'll use it.

   - if (!isDefinition || (NewTag != TTK_Class && NewTag != TTK_Struct))
   - if (OldTag == NewTag)
   - return true;
   + if (!isDefinition || !isClassCompatTagKind(NewTag) || OldTag == NewTag)
   + return true;

   The behaviour of the code is changed here, though I'm not sure if that has
   any impact on correctness.

It does - I must have not updated the patch after the last update I
made. In the later one this code is back to as it was except for adding
handling for __interface. [Oops. Grabbed the patch from the wrong
machine. Newer one is attached.]

   + default: assert("Invalid tag kind for field padding diagnostic!");

   You used this assert message on a couple diagnostic index helper functions
   which is incorrect for some. Seems to be a simple copy paste mismatch.

This is also taken care of in the attached update.

   + case TTK_Interface:
          Out << 'U';

   Did you confirm that cl mangles interfaces as structs?

Yes.

   + case TTK_Interface:
   + case TTK_Struct: return CXCursor_StructDecl;

   I think we should add a new cursor in the C API, and not re-use
   CXCursor_StructDecl for interfaces.

See previous message to list ("CXCursor value for __interface"): since
the cursor values are part of a public interface, I didn't think I could
change the values to insert an interface cursor; and there is no space
between CXCursor_LastDecl and CXCursor_FirstRef to add anything without
doing that. John McCall replied and said he thought using StructDecl was
fine.

__interface.patch (44.3 KB)

4

This seems like a real problem. The C API clients should have access to this information. Not having enough space seems like a real problem, especially since more declarations will probably need to be exposed in the future to libclang. John, what is the policy on binary compatibility?

5

For the C API, it’s important. I don’t know the role of the “max” constants, but I suspect that clients are meant to be conservative about cursor kinds that they don’t understand.

But I am not an expert in this API.

John.

6

The most obvious way to do this would be to add
  CD->getParent()->getTagKind() == TTK_Interface
to CXXMethodDecl::isVirtual().

John.