Page MenuHomePhabricator

[OpenCL] Added parsing for OpenCL vector types.
ClosedPublic

Authored by echuraev on Mar 21 2017, 5:22 AM.

Diff Detail

Event Timeline

echuraev created this revision.Mar 21 2017, 5:22 AM
Anastasia edited edge metadata.Mar 22 2017, 9:27 AM

Although nothing wrong with the implementation apart from it complicates a bit the understanding of conventional C parse flow by adding extra corner case, I don't see anything in the spec that states explicitly how the vector components should be parsed. I guess with extra parenthesis the parsing problem can easily be avoided:

((int3)(32, 32, 32)).x
test/Parser/vector-cast-define.cl
12

I would prefer to simply the test and avoid using macros... it will be easier for us to maintain it in the future.

I think this is a good feature for the convenience of user. I've seen usage like this.

I think this is a good feature for the convenience of user. I've seen usage like this.

I agree. I don't see any reasons why this case doesn't have the right to exist. I don't think that using extra parenthesis is a good solution for solving this problem.

I think this is a good feature for the convenience of user. I've seen usage like this.

I agree. I don't see any reasons why this case doesn't have the right to exist. I don't think that using extra parenthesis is a good solution for solving this problem.

I am just saying that I don't see a big use case for this. I am guessing it can largely come from the macro expansions, but those are generally good style to parenthesize.

I think this is a good feature for the convenience of user. I've seen usage like this.

I agree. I don't see any reasons why this case doesn't have the right to exist. I don't think that using extra parenthesis is a good solution for solving this problem.

I am just saying that I don't see a big use case for this. I am guessing it can largely come from the macro expansions, but those are generally good style to parenthesize.

Ok. But in current implementation if I forget to parenthesize the defined expression (as in the test) I will get the following message: "error: member reference base type 'int' is not a structure or union". I don't think that the message is clear to understand that you just forgot to add parenthesis.

So, should we change the diagnostic message to do it more understandable or push this patch because it can be more convenience for users?

I don't think that diagnostics can always be very clear. This is not the case neither for C nor C++.

As I said I don't see any issue to continue with this patch. I would just like to see the test simplified a bit.

echuraev updated this revision to Diff 93132.Mar 27 2017, 7:11 AM
echuraev marked an inline comment as done.
This revision is now accepted and ready to land.Mar 28 2017, 8:12 AM
echuraev closed this revision.Mar 28 2017, 10:20 PM