Community
Participate
Working Groups
Some of the AST node interfaces declare several int constants and also a special op_last constant, which is meant to allow extending interfaces to add additional constants. This approach is flawed, as shown by bug 162470. If there is more than one interface extending and they both use op_last+1 then you end up with values clashing. I believe the underlying issue is the use of ints to represent the constants. A potential solution would be to simply use Objects instead of ints. For example: public class OperatorProperty { // snip } interface IASTTypeIdExpression { public static final OperatorProperty op_sizeof = new OperatorProperty(); ... public OperatorProperty getOperator(); } interface ICPPASTTypeIdExpression extends IASTTypeIdExpression { public static final OperatorProperty op_typeid = new OperatorProperty(); } As long as each constant always creates a new object there won't be problems with clashing. Client code would use == to test for properties (although switches won't work anymore). OperatorProperty op = typeIdExpression.getOperator(); if(op == IASTTypeIdExpression.op_sizeof) { .... } I invite additional ideas for how to approach this problem.
The pragmatic solution is to define the constants in the base-interface (I have done so to fix bug 162470). There will not be an infinite number of those, so I don't think this should be a problem. Another approach can be to reserve ranges of constants for each extending interface in the base interface. Not being able to use switch-statements will make some of the code really ugly (e.g.: ExpressionWriter). It would also be a breaking change for the clients of the AST.
(In reply to comment #1) > The pragmatic solution is to define the constants in the base-interface (I have > done so to fix bug 162470). There will not be an infinite number of those, so I > don't think this should be a problem. This solution fixes bug 162470 but does not take into consideration the fact that op_last was there to allow extending interfaces to add constants. UPC adds three new kinds of sizeof expression: upc_localsizeof, upc_blocksizeof, and upc_elemsizeof. I was using op_last to define these constants but now its deprecated and we don't have a mechanism to reliably define these constants. I could just pick some arbitrary numbers but that would be a hack. We can't assume all the constants can just go in the base-interface, because this is public API and there may be language extensions in separate plugins. > Another approach can be to reserve ranges of constants for each extending > interface in the base interface. This is not possible because we cannot be aware of all possible language extensions. > Not being able to use switch-statements will make some of the code really ugly > (e.g.: ExpressionWriter). It would also be a breaking change for the clients of > the AST. I was talking to Chris about this and he suggested we use a mechanism for generating unique int values for the constants. I guess something like the following: public static final int op_sizeof = ConstantFactory.nextInt(); As long as the constant factory is global, synchronized and guaranteed to always return a unique value on every call it would work. There would be no API change (unless clients depend upon the specific values of the constants but in that case they deserve to be broken). This approach is very flexible, you can add a constant anywhere you like and not have to worry about it ever clashing with an existing value. Extending interfaces would have to be diligent enough to use the factory though. Markus what do you think? If you think this is too much fuss I can just use the hack solution and be done with it.
> I was talking to Chris about this and he suggested we use a mechanism for > generating unique int values for the constants. I guess something like the > following: > public static final int op_sizeof = ConstantFactory.nextInt(); > As long as the constant factory is global, synchronized and guaranteed to > always return a unique value on every call it would work. .... This will not work for any constants that are persisted in the index (e.g.: IBasicType.getType()). These need to be the same accross multiple sessions, even multiple installations (that can access the same workspace). In any way I think that clients of the AST have a right to know all the options. So I still recommend to add these constants into the appropriate interface in the core-plugin. The fact that this is API is no problem, it will not break any clients. Actually it is worse not defining the constants, because clients may still have to deal with the extra constants (without knowing them) when they for instance call IASTUnaryExpression.getOperator(). You probably also need to make changes to the core, where it relies on the operator constants, here some examples: * CVisitor.getExpressionType(IASTExpression), * CodeFormatterVisitor * ExpressionWriter * VariableReadWriteFlags
(In reply to comment #3) > This will not work for any constants that are persisted in the index (e.g.: > IBasicType.getType()). These need to be the same accross multiple sessions, > even multiple installations (that can access the same workspace). I was worried about that. Oh well. > In any way I think that clients of the AST have a right to know all the > options. I see your point. If clients always have to take into account the possibility of unknown extensions then that makes coding to the AST difficult. Besides, it doesn't work anyway, I had to masquerade UPC nodes as regular C nodes just to get binding resolution to work. > So I still recommend to add these constants into the appropriate > interface in the core-plugin. I don't think adding the UPC extension constants to the core interfaces is a good idea, if that's what you're suggesting. For gcc extensions its fine though because the core parser is supposed to be gcc compatible. > You probably also need to make changes to the core... I would rather not make changes to the core if it can be avoided. A better approach might be to just use op_sizeof for all the UPC sizeof operators. Then make the extra constants a separate thing, accessible via a getUPCSizeofOperator() property in the UPC node interfaces. This way clients that actually care which UPC operator was used can get at that information but otherwise CDT just treats them as regular C sizeof operators. This might be a better approach to extensibility in general. Instead of adding constants that would be accessed via an existing property like getOperator() just add a whole new property.
> I don't think adding the UPC extension constants to the core interfaces is a > good idea, if that's what you're suggesting. That's my suggestion. The AST is not part of the ANSI or GCC-parser it is used by all c/c++-parsers and needs to model all the options. > I would rather not make changes to the core if it can be avoided. > A better approach might be to just use op_sizeof for all the UPC sizeof > operators. Then make the extra constants a separate thing, accessible via a > getUPCSizeofOperator() property in the UPC node interfaces. This way clients > that actually care which UPC operator was used can get at that information but > otherwise CDT just treats them as regular C sizeof operators. > This might be a better approach to extensibility in general. Instead of adding > constants that would be accessed via an existing property like getOperator() > just add a whole new property. But then as a minimum the formatter the refactoring and the read/write flags of variables will not work properly with the upc-specific constructs.
(In reply to comment #5) > That's my suggestion. The AST is not part of the ANSI or GCC-parser it is used > by all c/c++-parsers and needs to model all the options. I still don't like it, but it may be a necessary evil. I guess language extensibility is hard so I may have no choice but to hack the core to get some stuff to work. I'm a committer so adding stuff to the core isn't an issue for me. But what about third parties that want to add language extensions to CDT, do they have to patch the core to get it to work? We need to favor solutions that improve extensibility rather than argue that extensibility is not possible. > But then as a minimum the formatter the refactoring and the read/write flags of > variables will not work properly with the upc-specific constructs. I need to improve my testing. Right now I'm not reusing all the tests so stuff like this isn't showing up. All the CDT JUnits, including the ones in the UI plugin, should be running against my parsers. But first the tests need to be made more reusable for this to work. Anyway, I've implemented the solution in my previous comment, for now. It may turn out to not be good enough.
