fix: reproducible standard library for clang-cl

[K-ballo]

fix #904

[cppalliance-bot]

An automated preview of the documentation is available at https://997.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://997.mrdocs.prtest2.cppalliance.org/index.html

[alandefreitas]

This will ultimately mean recreating each command in one of two versions, depending on the value of is_clang_cl.

Since adjustCommandLine is recreating the commands anyway, isn't it easier to just set the program name to "clang" instead of "progName" in std::vector new_cmdline = {progName}; and then never create clang-cl commands?

[alandefreitas]

This path is unnecessary. is_clang_cl would be generating behavior that's externally functionally equivalent for both true and false.

I know the point here is to make different values arrive at MrDocsCompilationDatabase, but that's not the role of SingleFileDB (which we might want to use independently later and not have to maintain both paths).

[K-ballo]

This is an attempt to represent a compilation database generated by cmake on windows.

[alandefreitas]

I understand that. Besides the obvious problem that clang-cl is not the only compiler on Windows, that's not the role of this class, and everything else I said above. This is not a test class, and you're including a change the class doesn't need. I left another comment explaining how you can test the change you actually want to test.

[K-ballo]

I don't follow, clang is not the only compiler on Windows/Linux either.

I agree that none of these hardcoded values belong in a class that represents a single file compilation database.

[alandefreitas]

This is an arbitrary compile value for a runtime condition. We just need to make sure the include paths we want are getting there. One easy way to test this is to define some macro or something in our libc stubs and check (maybe just static assert) if that's what we're getting in the libcxx.cpp test or some similar new test for libc stubs, or just for that.

Testing the same code with different is_clang_cl in the MrDocsCompilationDatabase is a bit harder, and we'd have to think about it. But that looks more like a unit test than a golden test. It should go into the unit tests, and we check if the commands have the properties we expect from them after MrDocsCompilationDatabase normalizes them.

[K-ballo]

The purpose of this change is to more closely match the actual user experience. It's a good idea to make the tests more robust.

While we own and can easily modify the libc stubs, there is no test for the system-libc option.

There is a test for the system-stdlib option, but those headers come directly from llvm-project and modifying them in place is questionable. We can add an extra file somewhere in build/share, include this location as an additional system-stdlib location, and check for it with __has_include.

The existing libcxx test check attempts to include <experimental/iterator> which is not currently provided by msvc, but there's no guarantee that it will never be provided.

[K-ballo]

Since adjustCommandLine is recreating the commands anyway, isn't it easier to just set the program name to "clang" instead of "progName" in std::vector new_cmdline = {progName}; and then never create clang-cl commands?

Simply changing the program name to "clang" doesn't work because the rest of the command line arguments (coming from the compilation database) are "cl" specific. For it to work, we would have to map each "cl" argument into its "clang" counterpart, and that is not always possible.

And it would cause clang to behave as clang, rather than emulate msvc. But perhaps there is a way to control this externally.

[alandefreitas]

Simply changing the program name to "clang" doesn't work because the rest of the command line arguments (coming from the compilation database) are "cl" specific.

That's not what I'm proposing. I'm proposing the name of the program will be clang in the output being generated by the function (the one you control), not the one you got as input (the one coming from the compilation database / that you got as input / the one you don't control).

For it to work, we would have to map each "cl" argument into its "clang" counterpart, and that is not always possible.

That is exactly what the MrDocsCompilationDatabase class is for. It's exactly what this function does. It has to be possible because we only have access to clang. If there's anything in a command that clang can't support, MrDocs can't support that either.

And it would cause clang to behave as clang, rather than emulate msvc.

Exactly. This reduces the number of possible problems.

[K-ballo]

That is exactly what the MrDocsCompilationDatabase class is for. It's exactly what this function does.

That's surprising, the function is tying to provide the correct "cl" vs "clang" argument depending on the emulation mode, rather than trying to map "cl" arguments to their "clang" counterparts.

It has to be possible because we only have access to clang.

We have access to clang, clang-cl, and whichever other modes clang supports.

Exactly. This reduces the number of possible problems.

I think removing msvc emulation is misguided. And it's more work than having support for both. I'll give it a try.