Output OpenCL version in Clang diagnostics

[rivanvx]

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.

[rivanvx]

Fixed wrong search and replace in tests.

[rivanvx]

Fix formatting.

[yaxunl]

LGTM

[Anastasia]

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]

Please, also add cfe-commits as Subscriber.

Thanks!
Anastasia

[rivanvx]

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

[Anastasia]

lib/Parse/ParseDecl.cpp

3516

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

[rivanvx]

lib/Parse/ParseDecl.cpp

3516

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

[rivanvx]

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

[Anastasia]

lib/Parse/ParseDecl.cpp

3516

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

Thanks!

[rivanvx]

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]

Make that int const as well.

[pxli168]

LGTM, thanks!

[Anastasia]

LGTM! Thanks!

[Anastasia]

Please, move cfe-commits to Subscribers list!

[rivanvx]

Thanks for the reviews!

[rivanvx]

Please, can anyone push this?

[Anastasia]

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.

[rivanvx]

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.

[Anastasia]

Sure. Committed in r269305!

Thanks!

[rivanvx]

@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]

In D19780#442305, @rivanvx wrote:

@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!

[Anastasia]

r269305