Page MenuHomePhabricator

[build.py] Add `VCINSTALLDIR` to default variables
ClosedPublic

Authored by aleksandr.urakov on Feb 6 2019, 1:07 AM.

Details

Summary

On my system, clang-cl can't compile tests containing char16_t and char32_t types without this environment variable defined. This fix makes the tests to pass, but I'm not sure about the such solution. What do you think about it?

Diff Detail

Repository
rL LLVM

Event Timeline

labath requested changes to this revision.Feb 6 2019, 1:53 AM

I don't think this should be necessary. Based on my experiments, the fact whether clang-cl defines char32_t depends on the version of visual studio it is emulating (-fms-compatibility-version). For instance I can do this:

$ cat /tmp/a.cc 
char32_t XX;
$ bin/clang-cl /tmp/a.cc /GS- /GR- /c -fms-compatibility-version=18
/tmp/a.cc(1,1) :  error: unknown type name 'char32_t'
char32_t XX;
^
1 error generated.
$ bin/clang-cl /tmp/a.cc /GS- /GR- /c -fms-compatibility-version=19
$ bin/clang-cl /tmp/a.cc /GS- /GR- /c

So, what I suspect is happening is that clang uses %VCINSTALLDIR% to detect the MSVC version, and then sets the compatibility based on that. If the detected version is high enough, char32_t will be automatically defined. The interesting part is that, for me, the default version seems to be high enough that even without any special setting char32_t "just works". However, I'm on linux, so the differences could be due to that.

So I think this is good news. I hope all that's needed here is to get build.py to add the -fms-compatibility thingy and things should work. Until we actually need to vary this version, I think we can just unconditionally add that flag to all clang-cl invocations in build.py.

This revision now requires changes to proceed.Feb 6 2019, 1:53 AM

I've checked your solution, and it's worked for me too! I think that your solution is better because it is more straight-forward. Here is the updated patch.

labath accepted this revision.Feb 6 2019, 4:59 AM

This looks good to me, but let's wait for zachary too.

I've checked your solution, and it's worked for me too! I think that your solution is better because it is more straight-forward. Here is the updated patch.

It also has a higher chance of behaving identically on different systems.

This revision is now accepted and ready to land.Feb 6 2019, 4:59 AM

Zachary, can you take a look? please?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 7:15 AM