Review Request: Parser support for C99 designated initializers

[Alexandre Courbot]

> On Dec. 19, 2011, 6:46 p.m., Milian Wolff wrote:
> >

Two short questions to help improving this patch:
1) about the use of rewind() in case of parse failure, is there a policy? Some functions do it, some others don't, and I cannot deduce clear rules for doing it in the code.
2) in case of parse failure, what happens to the AST elements that have been allocated midway? Existing code suggests that it is safe to just leave them this way and return false, but I have seen nothing that suggests these are collected later. Shouldn't them be freed?

I will try and come with a better version of this once I get answers to these. Looks like my AST construct is acceptable, which is a good point. Thanks for all the feedback so far!


> On Dec. 19, 2011, 6:46 p.m., Milian Wolff wrote:
> > languages/cpp/parser/parser.cpp, line 2752
> > <http://git.reviewboard.kde.org/r/103448/diff/1/?file=43720#file43720line2752>
> >
> >     this is bad, as it is *not* valid c++, see
> >     
> >     http://www.nongnu.org/hcb/#initializer-list
> >     
> >     please pass the mimetype along or find it again (in a fast way, I think the language controller can do that) to find out whether you are in "c" or in "c++" mode and decide based on that.

Is anything like this done to prevent C++11 constructs from being used in C++ files? If not, what is the point of being so strict about this very patch?

As David pointed there is no way to do that properly unless a C or C++ type is explicitly set for the file or project, something KDevelop does not support yet AFAIK. There are plenty, plenty of more serious things that the KDevelop parser accepts while gcc does not - i.e. incorrect type usage, non-matching number of arguments, etc. So unless we are aiming for a perfect C++ parser (in which case I would rather suggest to use LLVM/CLang's), let's not argue over a trailing comma.


> On Dec. 19, 2011, 6:46 p.m., Milian Wolff wrote:
> > languages/cpp/parser/parser.cpp, line 2845
> > <http://git.reviewboard.kde.org/r/103448/diff/1/?file=43720#file43720line2845>
> >
> >     was the change in order intentional? I'd rather have the parseDesignatedInitializer as last resort, as it will not occur in most code (as it's a C feature and we are currently mostly aiming at c++)

It is intentional. In the case of nested initializers, parseAssignmentExpression() would incorrectly take the nested one for some unknown reason. I can try to fix it and switch back to the previous order.


- Alexandre


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103448/#review9092
-----------------------------------------------------------


On Dec. 18, 2011, 12:23 p.m., Alexandre Courbot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103448/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2011, 12:23 p.m.)
> 
> 
> Review request for KDevelop and Milian Wolff.
> 
> 
> Description
> -------
> 
> Parser support for C99 designated initializers
> 
> Support C99 initializers in the C++ parser, e.g:
> 
> struct foo_t foo = {
>   .has_cake = true,
>   .nb_candles = 12,
> };
> 
> int bar[10] = {
>   [1] = 15,
>   [9] = 25,
> };
> 
> 
> Diffs
> -----
> 
>   languages/cpp/parser/parser.h ffc3967e9bec09ff56204aab98e8f80ec6b036cf 
>   languages/cpp/parser/parser.cpp 1c9d9e403500f35761ebc6deb737a4a68a53c28d 
>   languages/cpp/parser/tests/test_parser.h fa92f1ce284df0936724f74f42f9ad6d4b3c97fc 
>   languages/cpp/parser/tests/test_parser.cpp f747cfa44bdf962be33cf97841fdae739b3e1771 
> 
> Diff: http://git.reviewboard.kde.org/r/103448/diff/diff
> 
> 
> Testing
> -------
> 
> Used it for a couple of weeks, ensured the parser tests all pass.
> 
> 
> Thanks,
> 
> Alexandre Courbot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20111220/ea86ce15/attachment.html>