Page MenuHomePhabricator

Output OpenCL version in Clang diagnostics
ClosedPublic

Authored by rivanvx on May 1 2016, 3:42 PM.

Details

Summary

Whether OpenCL does or does not support the static storage class specifier and generic type qualifier is version dependent and diagnostics should note that.

This patch changes "OpenCL does not support" to "OpenCL version X.Y does not support" and also changes one message that contains "OpenCL2.0 version" to contain "OpenCL version 2.0" for consistent version reporting across all diagnostics including OpenCL version.

Diff Detail

Event Timeline

rivanvx updated this revision to Diff 55763.May 1 2016, 3:42 PM
rivanvx retitled this revision from to Output OpenCL version in Clang diagnostics.
rivanvx updated this object.
rivanvx added reviewers: yaxunl, Anastasia.
rivanvx updated this revision to Diff 55764.May 1 2016, 4:08 PM

Fixed wrong search and replace in tests.

rivanvx updated this revision to Diff 55784.May 1 2016, 10:55 PM

Fix formatting.

Anastasia added inline comments.May 6 2016, 4:53 AM
lib/Parse/ParseDecl.cpp
3514

Could we use StringRef instead to make this a big cleaner?

Perhaps, we could even use to_string instead of Twine...

Anastasia edited edge metadata.May 6 2016, 4:54 AM

Please, also add cfe-commits as Subscriber.

Thanks!
Anastasia

rivanvx updated this revision to Diff 56410.May 6 2016, 6:23 AM
rivanvx edited edge metadata.
rivanvx marked an inline comment as done.

Replace Twine with std::string and use llvm::to_string().

Anastasia added inline comments.May 6 2016, 7:58 AM
lib/Parse/ParseDecl.cpp
3516

I think it will be nicer to use string (not char*) here too.

rivanvx added inline comments.May 6 2016, 8:01 AM
lib/Parse/ParseDecl.cpp
3516

Other Spec are const char*, so I did it for consistency. But I don't care either way.

@Anastasia would you still prefer to make VerSpec a std::string?

Anastasia added inline comments.May 6 2016, 12:15 PM
lib/Parse/ParseDecl.cpp
3516

Yes, it will be shorter. StringRef should work too I think.

Thanks!

rivanvx updated this revision to Diff 56478.May 6 2016, 3:55 PM
rivanvx marked 3 inline comments as done.

I am neither aware how to convert ints to StringRef nor how to concatenate StringRefs. Apologies if I missed something in the API.

In any case, this approach looks pretty clean to me.

rivanvx updated this revision to Diff 56479.May 6 2016, 4:17 PM

Make that int const as well.

pxli168 accepted this revision.May 8 2016, 9:15 PM
pxli168 edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.May 8 2016, 9:15 PM
Anastasia accepted this revision.May 9 2016, 11:05 AM
Anastasia edited edge metadata.

LGTM! Thanks!

Please, move cfe-commits to Subscribers list!

rivanvx added a subscriber: cfe-commits.

Thanks for the reviews!

Please, can anyone push this?

Sure! Will do! Thanks!

I am thinking to factor out the version computation string into a common function, because we might use it in the other places too.

Could we solve that at a later point? There is one more place where such code is already used, but this would enlarge the scope of this patch.

If yes, I am wiling to factor it out after this is merged.

Sure. Committed in r269305!

Thanks!

@Anastasia I looked into introducing a separate getOpenCLVersion() function (or perhaps three - major version, minor version and version string). This would have to be used in lib/CodeGen/TargetInfo.cpp and lib/Parse/ParseDecl.cpp, and I am undecided on where should one put this code. One option would be in Parse/Parser.h inside class Parser, and then TargetInfo.cpp would have to include Parser.h, unless we decide to declare it inside AST/ASTContext.h.

In any case, this has so far two usages, and they are different (major and minor version in TargetInfo.cpp vs version string in ParseDecl.cpp). Therefore, I would propose to leave this as is for now, and rethink it after the same code has to be used in more places.

@Anastasia I looked into introducing a separate getOpenCLVersion() function (or perhaps three - major version, minor version and version string). This would have to be used in lib/CodeGen/TargetInfo.cpp and lib/Parse/ParseDecl.cpp, and I am undecided on where should one put this code. One option would be in Parse/Parser.h inside class Parser, and then TargetInfo.cpp would have to include Parser.h, unless we decide to declare it inside AST/ASTContext.h.

In any case, this has so far two usages, and they are different (major and minor version in TargetInfo.cpp vs version string in ParseDecl.cpp). Therefore, I would propose to leave this as is for now, and rethink it after the same code has to be used in more places.

Sure! It seems to make sense to me to have more use cases first. ;) Thanks for looking at it!