This is an archive of the discontinued LLVM Phabricator instance.

[Support] Class for response file expansion
ClosedPublic

Authored by sepavloff on Aug 22 2022, 8:17 AM.

Details

Summary

Functions that implement expansion of response and config files depend
on many options, which are passes as arguments. Extending the expansion
requires new options, it in turn causes changing calls in various places
making them even more bulky.

This change introduces a class ExpansionContext, which represents set of
options that control the expansion. Its methods implements expansion of
responce files including config files. It makes extending the expansion
easier.

No functional changes.

Diff Detail

Event Timeline

sepavloff created this revision.Aug 22 2022, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 8:17 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sepavloff requested review of this revision.Aug 22 2022, 8:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 22 2022, 8:17 AM

Can you say what you're trying to do?

Can you say what you're trying to do?

There is an intention to extend algorithm of included files search. In particular it requires passing down paths where config files may be searched for. It could solve the problem discussed in https://discourse.llvm.org/t/rfc-adding-a-default-file-location-to-config-file-support/63606, there are also other users that need such feature.

If config files are used to tune compiler for a platform which has variants, it is convenient to extract common platform settings into separate config file, which could be included by config files of particular variants. Now such solution does not work because included config files are searched differently than the file in --config option.

Move change for VFS support into separate patch

rnk added a comment.Sep 22 2022, 1:53 PM

This makes sense to me.

This probably affects nobody, but this reminds me of my first LLVM change: https://github.com/llvm/llvm-project/commit/fc8a2d5a8390952029e1c47a623e046b744f44d4

llvm/include/llvm/Support/CommandLine.h
2099

StringSaver is a stateless class that wraps a BumpPtrAllocator. To simplify the call site, I suggest making this parameter into a BumpPtrAllocator &, and construct a field StringSaver from the allocator.

2100

It feels like you are using the builder pattern to handle setting uncommon options. If that's what we're doing, should we commit completely and add a .setVFS() helper and remove the optional parameter?

sepavloff updated this revision to Diff 462528.Sep 23 2022, 9:37 AM

Address reviewer's notes

  • Pass BumpPtrAllocator instead of StringSaver,
  • Use setter to specify file system object.
sepavloff added inline comments.Sep 23 2022, 9:41 AM
llvm/include/llvm/Support/CommandLine.h
2099

Indeed in many cases StringSaver in caller can be omitted if the parameter is BumpPtrAllocator.

2100

You are right. Changed.

rnk accepted this revision.Sep 26 2022, 4:24 PM

lgtm

This revision is now accepted and ready to land.Sep 26 2022, 4:24 PM
This revision was automatically updated to reflect the committed changes.