NFC: Refactor clang extra args addition #1444
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@adrian-prantl please review |
| bool AddClangArgument(std::string arg, bool unique = true); | ||
|
|
||
| bool AddClangArgumentPair(llvm::StringRef arg1, llvm::StringRef arg2); | ||
|
|
| }; | ||
|
|
||
| inline bool IsMultiArgClangFlag(const StringRef& arg) { | ||
| return arg == "-D" || arg == "-U" || arg == "-working-directory"; |
There was a problem hiding this comment.
Previously we only had to check for multi-arg flags in the 3 cases we actually were doing something special to it. But with uniquing, do we need to check for more cases?
I guess we are still only uniquing the arguments which we recognize in GetClangArgCategory, right?
There was a problem hiding this comment.
For now that's true (except that it would be (GetClangArgCategory(arg) & ClangArgCategoryUnique) || arg == "-working-directory" as we use -working-directory only to apply to paths to make them absolute). In future there'll be at least -fmodule-map-file= which is single-arg by design, although it wouldn't break this method logic anyway. I could change this code to use output of GetClangArgCategory if you're okay with single-arg case.
There was a problem hiding this comment.
Actually, I've forgot to mention that there're different conditions: == vs startswith. Would you advise to separate some list of constants to unify lists of args to join/unique? I think of something like this:
const StringRef kMacroFlags[] = { "-D", "-U" };
const StringRef kUniqueMultiArgFlags[] = { "-I", "-F" }; // this would appear in the next PR, need an elegant way to append kMacroFlags here (using macro? :) )
And then just use these arrays to check for == or startswith where needed
There was a problem hiding this comment.
@adrian-prantl what do you think about this?
There was a problem hiding this comment.
Sure, that seems reasonable. The naming should follow the LLDB style though:
std::array<StringRef> macro_flags = {...};
std::array<StringRef> contractable_multiword_flags = {..};
| } // namespace | ||
|
|
||
| void SwiftASTContext::AddExtraClangArgs(std::vector<std::string> ExtraArgs) { | ||
| swift::ClangImporterOptions &importer_options = GetClangImporterOptions(); |
There was a problem hiding this comment.
This function has very few dependencies. I previously recommended writing an end-to-end test for the uniquing, but I wonder if we could write a unit test for it. That seems like a better fit.
There was a problem hiding this comment.
There's a test for -Werror skipping in this method, so I was hoping that it's possible to write more tests for logic here :)
There was a problem hiding this comment.
That would certainly work (we'd need to rename the test to be more generic), I just thought that this is such a nice and small piece of functionality that a unit test is a better fit. In the end, both variants are reasonable.
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
3 times, most recently
from
ec0ec92 to
13caaf9
Compare
adrian-prantl
approved these changes
Jul 14, 2020
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
from
13caaf9 to
3ed20f1
Compare
|
@swift-ci test |
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
2 times, most recently
from
60fc04a to
02cb77e
Compare
|
@swift-ci test |
|
|
||
| std::array<StringRef> macro_flags = { "-D", "-U" }; | ||
|
|
||
| bool IsMultiArgClangFlag(const StringRef& arg) { |
There was a problem hiding this comment.
Please use StringRef instead. StringRef only has a size of two pointers; it's designed to be passed by value.
|
|
||
| bool IsMacroDefinition(const StringRef& arg) { | ||
| for (auto &flag : macro_flags) | ||
| if (args.starswith(flag)) |
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
from
02cb77e to
1fed12a
Compare
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
2 times, most recently
from
ecde483 to
fbbd0ef
Compare
|
@swift-ci test |
Did you build this prior to uploading? |
|
Oh, sorry for that, I was too impatient for the last pushes, and incremental build doesn't work on my local checkout, so I have to wait about an hour to check compilation :( I'll make final push afther checking |
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
from
fbbd0ef to
155380d
Compare
|
@swift-ci test |
|
Sure! |
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
from
155380d to
5484364
Compare
|
@swift-ci test |
|
Thanks! |
Seems like you've made cherry-pick yourself, thanks! |
|
@DeeSee It looks like this patch introduced a use-after-free. Would you mind taking a look? https://ci.swift.org/view/LLDB/job/oss-lldb-asan-osx/6947/consoleText |
|
vedantk
reviewed
Jul 20, 2020
|
|
||
| auto clang_arg_str = clang_argument.str(); | ||
| if (!ShouldUnique(clang_argument) || !unique_flags.count(clang_arg_str)) { | ||
| importer_options.ExtraArgs.push_back(clang_arg_str); |
There was a problem hiding this comment.
@DeeSee doing importer_options.ExtraArgs.push_back invalidates all the references inside of llvm::DenseSet<StringRef> unique_flags. You might consider using llvm::StringSet.
There was a problem hiding this comment.
Thanks! I'll make a fix right away.
Just asking — why not std::unordered_set<std::string>? (I'm not very familiar with historical causes of why there are custom types for hashmap and hashset in llvm, and using types from std looks less error-prone to me)
There was a problem hiding this comment.
One (historical) reason is that LLVM predates std::unordered_set. However, in this concrete case llvm::StringSet will be more efficient than std::unordered_set<std::string> because it allocates the strings in the same memory as the set, so you don't have to pay extra memory management overhead for allocating the contents of the individual std::strings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AddClangArgumentPairmethodAddClangArgumentmethod (not used anywhere outsideSwiftASTContext.cpp)