NFC: Refactor clang extra args addition#1444
Merged
adrian-prantl merged 1 commit intoJul 17, 2020
Merged
Conversation
Author
|
@adrian-prantl please review |
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?
Author
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.
Author
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.
Sure, that seems reasonable. The naming should follow the LLDB style though:
std::array<StringRef> macro_flags = {...};
std::array<StringRef> contractable_multiword_flags = {..};
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.
Author
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
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
LGTM with inline comments addressed. Please do not forget to cherry-pick this to master-next!
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
Author
|
@swift-ci test |
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.
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
from
02cb77e to
1fed12a
Compare
Author
|
@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? |
Author
|
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 |
Author
|
Sure! |
DeeSee
force-pushed
the
refactor_clang_extra_flags
branch
from
155380d to
5484364
Compare
|
@swift-ci test |
|
Thanks! |
Author
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.
Author
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.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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)