-
Notifications
You must be signed in to change notification settings - Fork 22
fix: support clang-cl #1045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix: support clang-cl #1045
Conversation
src/test/TestRunner.cpp
Outdated
makeSingleFileDBForClang(filePath); | ||
#else | ||
makeSingleFileDBForClangCL(filePath); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still arbitrary.. there's no reason why both drivers couldn't be tested on every platform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
furthermore, this triggers unused function warnings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still arbitrary
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you suggest we run every test twice?
or should I rather conditionally define the one we're actually testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you suggest we run every test twice?
Sure. All platforms must be able to read both file formats, so it makes sense to test both formats on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to change the program name from "clang-cl" to just "cl", otherwise clang wasn't recognizing "/std:c++latest" on linux/macos. I do not have an explanation for this, why would it depend on the environment? @mizvekov ?
An automated preview of the documentation is available at https://1045.mrdocs.prtest2.cppalliance.org/index.html |
if (!isCCompileCommand) | ||
{ | ||
new_cmdline.emplace_back("-std=c++23"); | ||
new_cmdline.emplace_back(is_clang_cl ? "-std:c++latest" : "-std=c++23"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have a stable equivalent to latest
in the non-CL version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... OK. I see. You can calculate it with something like:
#include <clang/Basic/LangStandard.h>
#include <llvm/ADT/StringRef.h>
#include <string>
/// ...
static
std::string
latest_cxx_std_supported()
{
auto supports = [](llvm::StringRef name) {
const clang::LangStandard* LS = clang::LangStandard::getLangStandardForName(name);
return LS && LS->isCPlusPlus();
};
std::string cur = "c++23";
std::string last_ok = "c++23";
auto bump_by_3 = [](std::string& s) {
// s is "c++NN" with two decimal digits.
int n = (s[3] - '0') * 10 + (s[4] - '0');
n += 3;
s[3] = char('0' + (n / 10) % 10);
s[4] = char('0' + (n % 10));
};
for (;;) {
if (!supports(cur)) break;
last_ok = cur;
bump_by_3(cur);
}
return last_ok;
}
It also has the benefit of giving us the same version for CL and non-CL arguments in case the latest happens to have any special meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have complete control over clang, we can adjust this value manually when bumping llvm. If the target commit hash is exposed (-DLLVM_COMMIT_HASH=...
) we can even static assert on this, so it doesn't go undetected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can adjust this value manually when bumping llvm
Yes. We could. As you noticed, the problem is remembering. MrDocs has dozens, if not hundreds, of moving parts reflecting information back and forth. We always forget to update all these parts because no one remembers to do so. That's why things like the XML generator are broken.
we can even static assert on this
Exactly. For each type of information, we need a strategy that either forces us to review it because it's broken or that does it automatically.
Your solution is in the first category, and I do like it. There's no LLVM_COMMIT_HASH in their headers, but I'm guessing you would end up with something like:
#include <llvm/Config/llvm-config.h>
// ...
template <size_t N, size_t M>
consteval
bool
str_equal(const char (&a)[N], const char (&b)[M]) {
if (N != M) return false;
for (size_t i = 0; i < N; ++i)
if (a[i] != b[i]) return false;
return true;
}
static_assert(str_equal(LLVM_VERSION_STRING, "22.0.0git"),
"This code was designed with 22.0.0git in mind."
"Since you're moving to " LLVM_VERSION_STRING ", please: "
"1) review the code below and fix the c++ version if necessary"
"2) update the string in this assertion");
or
#include <llvm/Config/llvm-config.h>
// ...
static_assert(std::string_view(LLVM_VERSION_STRING) == "22.0.0git",
"This code was designed with 22.0.0git in mind."
"Since you're moving to " LLVM_VERSION_STRING ", please: "
"1) review the code below and fix the c++ version if necessary"
"2) update the string in this assertion");
I'm OK with some variant of that. All we have to be sure is it's always working. Not something the user can bypass by mistake.
By default, I usually prefer things in the second category whenever possible, but this hasn't been the case for every problem. For instance, if you look at the code I wrote, you'll see that I use "c++23"
as the baseline. The idea is that there's no cost if we forget to update it, but we wouldn't lose anything in terms of correctness if we did. There's probably some smart solution based on some enum or something somewhere.
Anyway, your macro based solution is also fine to me. I'm just explaining the rationale here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now #1047
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean simpler. I do agree they're more robust too.
An automated preview of the documentation is available at https://1045.mrdocs.prtest2.cppalliance.org/index.html |
An automated preview of the documentation is available at https://1045.mrdocs.prtest2.cppalliance.org/index.html |
An automated preview of the documentation is available at https://1045.mrdocs.prtest2.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2025-10-03 11:47:07 UTC |
fixes #904
continues the work from #997