sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks! just nits, apart from the "-target already explicitly set" issue Kadir raised. ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:99 + } + if (!Info.Target.empty()) + break; ---------------- I'm not sure these early returns are worth the complexity ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:102 + LLVM_FALLTHROUGH; + case IncludesExtracted: + if (Line.trim().startswith(TS)) { ---------------- also not sure whether initial vs includesextracted is worth distinguishing - this doesn't scale well if we add a third thing we're tracking (do we need a state for each combination that we could have seen already?) Maybe just a bool for SeenIncludes or something to detect duplicate/missing sections would be simpler ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:106 + TargetLine.consume_front(TS); + // Use only valid target + if (!isValidTarget(TargetLine)) { ---------------- this mostly echoes the code: "Only detect targets that clang understands"? ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:108 + if (!isValidTarget(TargetLine)) { + vlog("Invalid target \"{0}\", ignoring", TargetLine); + break; ---------------- I'd prefix with "System include extraction: " and bump up to elog - this is fairly likely to break things (and we'll only log it ~once thanks to caching) ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:112 + Info.Target = TargetLine.str(); + vlog("Target extracted: \"{0}\"", TargetLine); + } ---------------- also add a prefix here so it's clear what the context is in the log ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:227 + if (!Info->SystemIncludes.empty()) + log("System includes extractor: successfully executed {0}, got includes: " + "\"{1}\"", ---------------- we want to log this even if the set is empty, so the reader can tell the extraction worked (it's fine to only optionally log target) ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:231 + if (!Info->Target.empty()) + log("Target extractor: successfully executed {0}, got target: " + "\"{1}\"", ---------------- The target extractor isn't a separate component from the system includes extractor, so the log message seems a bit misleading. I'd suggest just using "System includes extractor" for both, even if it's not totally accurate ================ Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:212 + if (!Target.empty()) { + Cmd.CommandLine.push_back("-target"); + Cmd.CommandLine.push_back(Target); ---------------- kadircet wrote: > ArcsinX wrote: > > ArcsinX wrote: > > > kadircet wrote: > > > > sammccall wrote: > > > > > `clang -target foo test.cc` seems to be a hard error in the driver if > > > > > the target is unknown. > > > > > (vs likely *some* functionality if we just didn't set the driver) > > > > > > > > > > so this could regress some scenarios. Can we mitigate that? > > > > > (It's possible that we're running the driver in a mode where we > > > > > proceed anyway, but I can't remember :-() > > > > what if target already exists in `Cmd`? > > > > > > > > also it would be nice to use `--target=X` format to be consistent with > > > > target inference from invocation name as in > > > > https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Tooling.cpp#L278. > > > I failed to find options to process in case of invalid target, so added > > > target validation. > > We could specify several --target options, the last one will be used. But I > > am not sure should we override existing or not. > > We could specify several --target options, the last one will be used. But I > > am not sure should we override existing or not. > > Having multiple targets sounds confusing. I would prefer keeping a target > specifically mentioned by the user but it is also possible for the target to > be inferred by clang (and that might be wrong). This is similar to our stand > against predefined macros though, i think we should fix clang's inference > logic if it has any false positives. So i am slightly leaning towards not > overriding the target if it exists. I think the question of "target already explicitly set" should probably be answered. Checking for an arg equal to "-target" or beginning with "--target" is probably enough. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92012/new/ https://reviews.llvm.org/D92012 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits