[Macros] Plugin search options group #66650
Conversation
rintaro
force-pushed
the
macros-plugins-searchopts
branch
from
f18d241 to
090fcda
Compare
There was a problem hiding this comment.
ModuleSearchPathKind::CompilePlugin was incomplete (only set for -load-plugin-library), and was not actually used.
lib/AST/ASTContext.cpp
Outdated
There was a problem hiding this comment.
Ditto: SearchPathKind::CompilerPlugin wasn't actually used.
There was a problem hiding this comment.
Plugin search doesn't use Lookup, and adding plugin search path won't affect import module search path. So we don't need to call Lookup.searchPathsDidChange() when updating PluginSearchOpts.
lib/AST/PluginLoader.cpp
Outdated
There was a problem hiding this comment.
Currently we create a lookup table for -load-plugin-executable only. But we could lazily create a lookup table for all keyed by module names. But that's need to scan (iterate directory entries) all search paths, so I'm not sure it worth it.
There was a problem hiding this comment.
It's probably better to do the one scan rather than the continual exists checks, but not doing that in this PR is fine IMO.
There was a problem hiding this comment.
Previously manually specified -plugin-path was duplicated in libDriver.
'load-plugin-library', 'load-plugin-executable', '-plugin-path' and '-external-plugin-path' should be searched in the order they are specified in the arguments. Previously, for example '-plugin-path' used to precede '-external-plugin-path' regardless of the position in the arguments.
rintaro
force-pushed
the
macros-plugins-searchopts
branch
from
090fcda to
5791a2c
Compare
| pair.ModuleNames, [&](auto &name) { optStr += name; }, | ||
| [&]() { optStr += ","; }); | ||
| serializationOpts.CompilerPluginExecutablePaths.push_back(optStr); | ||
| // FIXME: Preserve the order of these options. |
There was a problem hiding this comment.
Are you intentionally trying to avoid changing the module serialization format here?
There was a problem hiding this comment.
Yes, my intension is doing it in a followup.
|
@swift-ci Please smoke test |
lib/AST/PluginLoader.cpp
Outdated
There was a problem hiding this comment.
It's probably better to do the one scan rather than the continual exists checks, but not doing that in this PR is fine IMO.
| case OPT_external_plugin_path: { | ||
| // '<plugin directory>#<plugin server executable path>'. | ||
| // FIXME: '#' can be used in the paths. | ||
| StringRef dylibPath; |
| // RUN: -o %t/main2 \ | ||
| // RUN: -module-name test \ | ||
| // RUN: -load-plugin-executable %t/libexec/MacroDefinitionPlugin#MacroDefinition \ | ||
| // RUN: -plugin-path %t/lib/plugins \ |
There was a problem hiding this comment.
Add -load-plugin-library here too?
| // RUN: -external-plugin-path %t/external#%swift-plugin-server \ | ||
| // RUN: -plugin-path %t/lib/plugins \ | ||
| // RUN: -load-plugin-executable %t/libexec/MacroDefinitionPlugin#MacroDefinition \ | ||
| // RUN: -o %t/main4 |
|
swiftlang/llvm-project#6998 |
rintaro
requested review from
ahoppen,
artemcm,
hborla,
slavapestov and
xedin
as code owners
3 participants
load-plugin-library,load-plugin-executable,-plugin-pathand-external-plugin-pathshould be searched in the order they are specified in the arguments.Previously, for example
-plugin-pathused to precede-external-plugin-pathregardless of the position in the arguments.