This is an archive of the discontinued LLVM Phabricator instance.

[clang-offload-bundler] use std::forward_list for storing temp file names [NFC]
ClosedPublic

Authored by sdmitriev on Nov 24 2020, 1:08 AM.

Details

Summary

Use a different container that preserves existing elements on modification
for storing temporary file names. Current container can make StringRefs
returned earlier invalid on reallocation.

Diff Detail

Unit TestsFailed

Event Timeline

sdmitriev created this revision.Nov 24 2020, 1:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2020, 1:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sdmitriev requested review of this revision.Nov 24 2020, 1:08 AM
ABataev added inline comments.Nov 24 2020, 5:00 AM
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
411

What about llvm::iplist?

sdmitriev added inline comments.Nov 24 2020, 6:55 AM
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
411

It can be changed to iplist as well, but why do you think it is a better option here?

ABataev added inline comments.Nov 24 2020, 6:57 AM
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
411

Just better to use LLVM API if possible.

sdmitriev added inline comments.Nov 24 2020, 7:35 AM
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
411

Well, I do not think it will bring any benefits here compared to the forward_list. iplist is an intrusive list, so it requires an element to provide access to prev/next elements in the list, and because of that element type should be derived from ilist_node. So, switching to iplist would require adding one more class for the list element type. I agree that it is not difficult to do, but I just do not see why it needs to be done.

This revision is now accepted and ready to land.Nov 24 2020, 7:43 AM
This revision was landed with ongoing or failed builds.Nov 24 2020, 8:09 AM
This revision was automatically updated to reflect the committed changes.