This is an archive of the discontinued LLVM Phabricator instance.

[NFC][clang-tools-extra]Replace find/insert with try_emplace
AcceptedPublic

Authored by prehistoric-penguin on Aug 2 2022, 11:13 PM.

Details

Summary

The change will save us one find call in map, which is more efficient and
more clear.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 11:13 PM
prehistoric-penguin requested review of this revision.Aug 2 2022, 11:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 11:13 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
avogelsgesang added inline comments.Aug 3 2022, 12:49 AM
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
173

Previously, we had an additional check It->second != SourceTU here. I don't understand what this check was doing, but this does not seem to be a pure refactoring.

If this is still an NFC change, please update the commit message to explain why this condition was unneeded

Update commit message

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
173

Thanks! I have reviewed the change and fixed it, now the logic is aligned

prehistoric-penguin retitled this revision from [clang-tools-extra]Replace find/insert with try_emplace to [NFC][clang-tools-extra]Replace find/insert with try_emplace.Aug 3 2022, 1:08 AM
avogelsgesang accepted this revision.Aug 3 2022, 1:12 AM

The change itself looks good.

Please look into the CI failures before landing this.

This revision is now accepted and ready to land.Aug 3 2022, 1:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2022, 1:30 AM
prehistoric-penguin added a comment.EditedAug 3 2022, 2:44 AM

CI failed since LLVM is still using c++14 to build

CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/clang-apply-replacements -I/var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements -I/var/lib/buildkite-agent/builds/llvm-project/clang/include -I/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/include -I/var/lib/buildkite-agent/builds/llvm-project/build/include -I/var/lib/buildkite-agent/builds/llvm-project/llvm/include -I/var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements/include -gmlt -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/clang/tools/extra/clang-apply-replacements/CMakeFiles/obj.clangApplyReplacements.dir/lib/Tooling/ApplyReplacements.cpp.o -MF tools/clang/tools/extra/clang-apply-replacements/CMakeFiles/obj.clangApplyReplacements.dir/lib/Tooling/ApplyReplacements.cpp.o.d -o tools/clang/tools/extra/clang-apply-replacements/CMakeFiles/obj.clangApplyReplacements.dir/lib/Tooling/ApplyReplacements.cpp.o -c /var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

/var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:171:35: error: no member named 'try_emplace' in 'std::map<clang::tooling::Replacement, const clang::tooling::TranslationUnitDiagnostics *>'

        auto InsertPos = Replaces.try_emplace(R, SourceTU);

We may close this temporarily and reopen it after the c++17 flag is ready.

Or we may switch the type of DiagReplacements from llvm::DenseMap<const FileEntry *,std::map<tooling::Replacement, const tooling::TranslationUnitDiagnostics *>> to llvm::DenseMap<const FileEntry *,llvm::DenseMap<tooling::Replacement, const tooling::TranslationUnitDiagnostics *>>