Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support C99 compound literals. #237

Merged
merged 1 commit into from
Jul 10, 2014

Conversation

francoisferrand
Copy link
Contributor

ISO C99 supports compound literals. A compound literal looks like a cast
containing an initializer. Its value is an object of the type specified
in the cast, containing the elements specified in the initializer; it is
an lvalue.

For example,

struct foo {int a; char b[2];} structure;
structure = ((struct foo) {x + y, 'a', 0});

@wenns
Copy link
Contributor

wenns commented Jun 29, 2014

This is not valid c++, is it? The goal is to support C++11 and following. I dont think it is wise to throw together all syntaxes we find in the wild... I suppose this road doesnt lead nowhere.

@francoisferrand
Copy link
Contributor Author

The thing is the plugins gets both C and C++ files. While the majority of C files (esp. legacy C files) parse just fine with the C++ parser, C99 added some extra syntax which have unfortunately not been added to C++11...

Without this patch, many C99 files fail to parse, and checkers cannot be used. And it seems GCC supports this for C++ as well.

@guwirth
Copy link
Collaborator

guwirth commented Jun 30, 2014

http://stackoverflow.com/questions/3869963/compound-literals-in-msvc

The construct (Type){initialisers} is not a cast operation, but it is the syntactic construct of a compound literal. This is a C99 construct, which GCC also supports in its C++ compiler as an extension. As far as I can determine, compound literals are not supported by MSVC in either its C or C++ mode.

Good description about that topic: http://en.m.wikipedia.org/wiki/Compatibility_of_C_and_C%2B%2B

  • In general we have to decide: Is plugin for C, C++ or both? We had this discussion already several times.
  • Commercial C++ plugin has a switch:
    • Possible values for sonar.c.std are: “c89″ and “c99″, and “c11″, which is the default.
    • Possible values for sonar.cpp.std are: “c++03″ and “c++11″, which is the default.
  • I would tend to be able to parse as much as possible without any switches. There might be some cases left where syntax of C and C++ is different. As long as AST is not used and only external tools are used this doesn't matter.

@wenns
Copy link
Contributor

wenns commented Jun 30, 2014

@Typz: Now that we have the multi-language-feature, its is not difficult to configure things so such that one plugin gets only c++ sources and c-ones go to another. The only thing you have to make sure is different extensions for different languages. Which should be doable. So thats not a reason.
As for GCC support: I can compile the example line with 'gcc' but not with 'g++'. So it is supported by the C-compiler only.

@guwirth: Yes, we have to decide. And I have a problem with mixing it all -- various C and C++ standards and various extensions from different compiles -- in one grammar because I cannot estimate well where we will land. The standard syntax is already f_cked up enough... and has its ambiguities (which we still arent able to manage). Im not sure its wise to make it even worse by trying to support more syntaxes. And -- an important point -- its wrong that the AST doesnt matter. We already use it for a couple metrics and we will use it even more. The AST *matters_.

On the other hand, I understand the (very pragmatic) motivation to support as much code as possible with this grammar.

BTW, a quote from the page you posted:
Several additions of C99 are or were not supported in C++ or conflicted with C++ features, such as...

Hmm...

@francoisferrand
Copy link
Contributor Author

I understand the theory; the only problem is there is no other C plugin (except the C/C++ plugin from sonarqube)... And forking the C++ plugin would duplicate quite a bit of work (e.g. all the checkers and parsers).

Maybe a solution would be to flag the differences, and instance 2 parser objects, with different flags.

On the other hand, these feature are starting to make C++ incompatible with C, which i am not sure is so desirable. Can't do much about it, and that is just my humble opinion. But with this "vision" of the 2 languages getting compatible eventually I would prefer to keep as single parser.

note: sorry about the incorrect info regarding GCC support, I did not actually try it, just read some user reports. Btw Clang is supposed to implement it for c++ as we'll -- again, from user reports, to be confirmed.

On 30 juin 2014, at 21:15, Waleri Enns notifications@github.com wrote:

@Typz: Now that we have the multi-language-feature, its is not difficult to configure things so such that one plugin gets only c++ sources and c-ones go to another. The only thing you have to make sure is different extensions for different languages. Which should be doable. So thats not a reason.
As for GCC support: I can compile the example line with 'gcc' but not with 'g++'. So it is supported by the C-compiler only.

@guwirth: Yes, we have to decide. And I have a problem with mixing it all -- various C and C++ standards and various extensions from different compiles -- in one grammar because I cannot estimate well where we will land. The standard syntax is already f*cked up enough... and has its ambiguities (which we still arent able to manage). Im not sure its wise to make it even worse by trying to support more syntaxes. And -- an important point -- its wrong that the AST doesnt matter. We already use it for a couple metrics and we will use it even more. The AST matters.

On the other hand, I understand the (very pragmatic) motivation to support as much code as possible with this grammar.

BTW, a quote from the page you posted:
Several additions of C99 are or were not supported in C++ or conflicted with C++ features, such as...

Hmm...


Reply to this email directly or view it on GitHub.

@guwirth
Copy link
Collaborator

guwirth commented Jul 2, 2014

the only problem is there is no other C plugin

Commercial one is also supporting both in one plugin.

I have a problem with mixing it all -- various C and C++ standards and various extensions from different compiles -- in one grammar

As long as there are no ambiguities by the extensions I would add it. Later one we could still split it into different grammars if needed. How is commercial plugin doing this? Detection by file extension?

its wrong that the AST doesnt matter.

I mean: It doesn't matter if only external tools are used.

We already use it for a couple metrics and we will use it even more. The AST matters.

I'm not sure if this is the right way or we should not better rely on external tools. Looking to the current checks they are all very special or existing static code analyzes are doing the same.

@wenns
Copy link
Contributor

wenns commented Jul 2, 2014

On 07/02/2014 09:04 PM, Günter Wirth wrote:

the only problem is there is no other C plugin

Commercial one is also supporting both in one plugin.

I have a problem with mixing it all -- various C and C++ standards
and various extensions from different compiles -- in one grammar

As long as there are no ambiguities by the extensions I would add it.
Later one we could still split it into different grammars if needed. How
is commercial plugin doing this? Detection by file extension?

its wrong that the AST doesnt matter.

I mean: It doesn't matter if only external tools are used.

We already use it for a couple metrics and we will use it even more.
The AST matters.

I'm not sure if this is the right way or we should not better rely on
external tools. Looking to the current checks they are all very special
or existing static code analyzes are doing the same.

Im actually not talking about the builtin checks, but all features which
are based on the AST. This includes most of the size-like metrics
(#locs, #statments/classes/methods), complexity, custom XPath rules,
design metrics, public API detection etc.

And more is coming: all the cartography-stuff which from SonarSource's
roadmap requires an accurate program model which in turn has to build
upon the AST.

Parsing and building the AST doesnt end in itself, it enables many
things, many of which we get 'for free' through the usage of the SQ
platform or SQ libs. When the goal is to achieve accurate measures, an
accurate and 'high quality' AST is a precondition. Or at least I believe
so. The 'just get it all parsed'-goal is probably somewhat short
sighted, a point may come when we will be thinking about the
size/shape/'quality' of our AST...

@wenns
Copy link
Contributor

wenns commented Jul 7, 2014

All right guys... Lets put users needs in the foreground :)

I'll merge the C-compatibility PR series. For PR's of this kind I'll ask for complete test coverage, both on unit and integration level. Im asking for this because its critical to encounter conflicts and ambiguities as early as possible and such additions have a high potential for causing them.

The grammar changes and according unit tests should be marked with comments (something like: //C-COMPATIBILITY: xxxxx would suffice). This will be important when we're forced to split the current grammar in multiple grammars.

For this concrete PR, we have to add:

  • unit test(s)
  • a couple of comments

And Im hoping for not-that-many patches of this kind ;)

ISO C99 supports compound literals. A compound literal looks like a cast
containing an initializer. Its value is an object of the type specified
in the cast, containing the elements specified in the initializer; it is
an lvalue.

For example,
    struct foo {int a; char b[2];} structure;
    structure = ((struct foo) {x + y, 'a', 0});
wenns added a commit that referenced this pull request Jul 10, 2014
Support C99 compound literals.
@wenns wenns merged commit 3ea4b14 into SonarOpenCommunity:master Jul 10, 2014
@francoisferrand francoisferrand deleted the compoundliterals branch August 11, 2014 10:44
@guwirth
Copy link
Collaborator

guwirth commented Dec 31, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants