Skip to content
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

NFC: Refactor clang extra args addition #1444

Merged
merged 1 commit into from Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 39 additions & 47 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp
Expand Up @@ -1448,39 +1448,66 @@ void ApplyWorkingDir(SmallString &clang_argument, StringRef cur_working_dir) {
llvm::sys::path::append(clang_argument, cur_working_dir, rel_path);
llvm::sys::path::remove_dots(clang_argument);
}

std::array<StringRef, 2> macro_flags = { "-D", "-U" };

bool IsMultiArgClangFlag(StringRef arg) {
for (auto &flag : macro_flags)
if (flag == arg)
return true;
return arg == "-working-directory";
}

bool IsMacroDefinition(StringRef arg) {
for (auto &flag : macro_flags)
if (arg.startswith(flag))
return true;
return false;
}

bool ShouldUnique(StringRef arg) {
return IsMacroDefinition(arg);
}
} // namespace

void SwiftASTContext::AddExtraClangArgs(std::vector<std::string> ExtraArgs) {
swift::ClangImporterOptions &importer_options = GetClangImporterOptions();
llvm::DenseSet<StringRef> unique_flags;
for (auto &arg : importer_options.ExtraArgs)
unique_flags.insert(arg);

llvm::SmallString<128> cur_working_dir;
llvm::SmallString<128> clang_argument;
for (const std::string &arg : ExtraArgs) {
// Join multi-arg -D and -U options for uniquing.
// Join multi-arg options for uniquing.
clang_argument += arg;
if (clang_argument == "-D" || clang_argument == "-U" ||
clang_argument == "-working-directory")
if (IsMultiArgClangFlag(clang_argument))
continue;

auto clear_arg = llvm::make_scope_exit([&] { clang_argument.clear(); });

// Enable uniquing for -D and -U options.
bool is_macro = (clang_argument.size() >= 2 && clang_argument[0] == '-' &&
(clang_argument[1] == 'D' || clang_argument[1] == 'U'));
bool unique = is_macro;

// Consume any -working-directory arguments.
StringRef cwd(clang_argument);
if (cwd.consume_front("-working-directory")) {
cur_working_dir = cwd;
continue;
}
// Drop -Werror; it would only cause trouble in the debugger.
if (clang_argument.startswith("-Werror")) {
if (clang_argument.startswith("-Werror"))
continue;
}

if (clang_argument.empty())
continue;

// Otherwise add the argument to the list.
if (!is_macro)
if (!IsMacroDefinition(clang_argument))
ApplyWorkingDir(clang_argument, cur_working_dir);
AddClangArgument(clang_argument.str(), unique);

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeeSee doing importer_options.ExtraArgs.push_back invalidates all the references inside of llvm::DenseSet<StringRef> unique_flags. You might consider using llvm::StringSet.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

unique_flags.insert(clang_arg_str);
}
}
}

Expand Down Expand Up @@ -3387,41 +3414,6 @@ swift::DWARFImporterDelegate *SwiftASTContext::GetDWARFImporterDelegate() {
return m_dwarf_importer_delegate_up.get();
}

bool SwiftASTContext::AddClangArgument(std::string clang_arg, bool unique) {
if (clang_arg.empty())
return false;

swift::ClangImporterOptions &importer_options = GetClangImporterOptions();
// Avoid inserting the same option twice.
if (unique)
for (std::string &arg : importer_options.ExtraArgs)
if (arg == clang_arg)
return false;

importer_options.ExtraArgs.push_back(clang_arg);
return true;
}

bool SwiftASTContext::AddClangArgumentPair(StringRef clang_arg_1,
StringRef clang_arg_2) {
if (clang_arg_1.empty() || clang_arg_2.empty())
return false;

swift::ClangImporterOptions &importer_options = GetClangImporterOptions();
bool add_hmap = true;
for (ssize_t ai = 0, ae = importer_options.ExtraArgs.size() -
1; // -1 because we look at the next one too
ai < ae; ++ai) {
if (clang_arg_1.equals(importer_options.ExtraArgs[ai]) &&
clang_arg_2.equals(importer_options.ExtraArgs[ai + 1]))
return false;
}

importer_options.ExtraArgs.push_back(clang_arg_1);
importer_options.ExtraArgs.push_back(clang_arg_2);
return true;
}

const swift::SearchPathOptions *SwiftASTContext::GetSearchPathOptions() const {
VALID_OR_RETURN(0);

Expand Down
4 changes: 0 additions & 4 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.h
Expand Up @@ -215,10 +215,6 @@ class SwiftASTContext : public TypeSystemSwift {

bool AddModuleSearchPath(llvm::StringRef path);

bool AddClangArgument(std::string arg, bool unique = true);

bool AddClangArgumentPair(llvm::StringRef arg1, llvm::StringRef arg2);

/// Add a list of Clang arguments to the ClangImporter options and
/// apply the working directory to any relative paths.
void AddExtraClangArgs(std::vector<std::string> ExtraArgs);
Expand Down