[PATCH] D123837: [C++20] [Modules] Judge isInCurrentModule currently
Iain Sandoe via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 19 03:40:04 PDT 2022
iains added a comment.
it would be good to add tests for GMF and PMF cases?
================
Comment at: clang/include/clang/Basic/Module.h:550-553
if (isModulePartition()) {
auto pos = Name.find(':');
return StringRef(Name.data(), pos);
}
----------------
if we find this is repeated too many times, we could cache it in the module as PrimaryModuleName.
================
Comment at: clang/include/clang/Basic/Module.h:537-543
+ static StringRef getPrimaryModuleInterfaceName(StringRef Name) {
+ return Name.split(':').first;
+ }
+
/// Get the primary module interface name from a partition.
StringRef getPrimaryModuleInterfaceName() const {
+ return getPrimaryModuleInterfaceName(getTopLevelModuleName());
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > I do not understand the purpose of this change, we already iterated over making the function more efficient.
> > > >
> > > > For C++20 modules there cannot be more than one parent - so that the nested call to getTopLevelModule(), via getTopLevelModuleName() is redundant.
> > > >
> > > > Is there some other case that needs special handling?
> > > > (If so then I think we should guard this at a higher level)
> > > >
> > > The primary purpose of the change is that we need to get the primary module name for getLangOpts().CurrentModule. So we need a static member function which takes a StringRef.
> > >
> > > Another point might be the correctness. For example, for the private module fragment, the current implementation would return the right result while the original wouldn't. (Although we didn't use private module fragment much...)
> > >
> > > For the most case, I admit the current implementation would call more function a little more. But I think it is negligible.
> > (Maybe I am worrying too much - but it does seem that we have quite some complexity in an operation that we expect to repeat many times per TU).
> >
> > ----
> >
> > > The primary purpose of the change is that we need to get the primary module name for getLangOpts().CurrentModule. So we need a static member function which takes a StringRef.
> >
> > That is, of course, fine (and we are stuck in the case that we are building a partition, with the fact that the primary module is not available). It seems that there's not a lot we can do there.
> >
> > When we have module, we could cache the result of the split, tho - so have a "PrimaryModuleName" entry in each module and populate it on the first use. We are still stuck with the string comparison - but at least we could avoid repeating the split?
> > As I noted, the reason we did not bother to do this in the first use was that that use was only once per TU).
> >
> > When we are building a consumer of a module, on the other hand, we *do* have the complete module import graph.
> >
> > > Another point might be the correctness. For example, for the private module fragment, the current implementation would return the right result while the original wouldn't. (Although we didn't use private module fragment much...)
> >
> > Of course, we must fix correctness issues ..
> > Since there is only GMF and PMF, perhaps we could reasonably treat those specially `if (isGlobalModule())` .. and/or `if (isPrivateModule())`?
> >
> > > For the most case, I admit the current implementation would call more function a little more. But I think it is negligible.
> >
> > It is hard to judge at the moment - perhaps we should carry on with what we have and then instrument some reasonable body of code?
> > (Maybe I am worrying too much - but it does seem that we have quite some complexity in an operation that we expect to repeat many times per TU).
>
> Never mind! This is the meaning of reviewing.
>
> > Of course, we must fix correctness issues ..
> Since there is only GMF and PMF, perhaps we could reasonably treat those specially if (isGlobalModule()) .. and/or if (isPrivateModule())?
>
> Done.
>
> After consideration, I think what you here makes sense. Especially now Module::getPrimaryModuleInterfaceName is only used for Sema.getLangOpts().CurrentModule. So it might be better to handle this in Sema or LangOptions.
> Especially now Module::getPrimaryModuleInterfaceName is only used for Sema.getLangOpts().CurrentModule. So it might be better to handle this in Sema or LangOptions.
Yeah, perhaps cache that split in Sema - adding another LangOption does not feel quite right (unless we find we need this information more widely - but I didn't see that yet).
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1567-1568
+
+ return M->getPrimaryModuleInterfaceName() ==
+ Module::getPrimaryModuleInterfaceName(LangOpts.CurrentModule);
}
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > ... of course, "correctness before optimisation", but this test function seems like it would benefit from some refactoring as suggested below.
> > > >
> > > > it seems a bit unfortunate that we are placing a string comparison in the "acceptable decl" lookup path, which might be made many times (the previous use of the string compare was in making the module decl - which only happens once in a TU).
> > > >
> > > > * Sema has visibility of the import graph of modules (via DirectModuleImports and Exports).
> > > > * We also know what the module parse state is (FirstDecl, GMF etc)
> > > > * If were are not parsing a module (i.e. LangOpts.CurrentModule.empty()) the we could return false early?
> > > > * if ModuleScopes is empty we know we are not (yet) parsing a module
> > > > * Once we are parsing a module then we can test ModuleScopes.back() to find the current module
> > > >
> > > > there are only two users of isInCurrentModule() - so maybe we refactor could make more use of the higher level state information - and / or cache information on the parents of partitions to avoid the complexity of parsing strings each time.
> > > >
> > > > what do you think?
> > > Yeah, string comparison is relatively expensive and we should avoid that. I added a search cache here and I thought it should handle the most cases.
> > >
> > > For the solution you described, I am not sure if I understood correctly. It looks like a quick return path if we found we are not parsing a module? If yes, I think the current check could do similar works.
> > That looks good.
> >
> > On the actual test itself:
> >
> > 1/
> > we have the check of `(M->isGlobalModule() && !M->Parent)`
> >
> > So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
> > if `M->isGlobalModule() && M->Parent` ?
> > it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?
> >
> > 2/
> > When we are actually parsing the PMF we have a similar case, no?
> > (so that there is no Parent pointer set yet, and I think that means that we would need to examine ModuleScopes to find the Parent module?)
> >
> >
> > 1/
> > we have the check of (M->isGlobalModule() && !M->Parent)
> >
> > So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
> > if M->isGlobalModule() && M->Parent ?
> > it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?
>
> Good Catcha! Now in this revision, the function would return false for the case of `M->isGlobalModule() && M->Parent`. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name `isUsableModule` for it from the wording of `[module.global.frag]p1`. I tried to call it `isVisibleModule` but I found there is already one... I feel `isUsableModule` is not perfectly good... Any suggestion?
>
> > 2/
> > When we are actually parsing the PMF we have a similar case, no?
>
> There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.
> > 1/
> > we have the check of (M->isGlobalModule() && !M->Parent)
> >
> > So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
> > if M->isGlobalModule() && M->Parent ?
> > it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?
>
> Good Catcha! Now in this revision, the function would return false for the case of `M->isGlobalModule() && M->Parent`. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name `isUsableModule` for it from the wording of `[module.global.frag]p1`. I tried to call it `isVisibleModule` but I found there is already one... I feel `isUsableModule` is not perfectly good... Any suggestion?
actually that seems a reasonable name for now - I am beginning to feel that once we have things basically working - the whole module visibility stack could be refactored ...
> > 2/
> > When we are actually parsing the PMF we have a similar case, no?
>
> There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.
Yes, but....
a) When we are actually parsing the PMF, there will be no 'Parent' pointer set, so that we would have to find the parent some other way, right?
(perhaps it is not too complex - since we should already have diagnosed a bad parse situation if we see a :private module line in the wrong place - so that all we need to be able to do is to locate the current module - that **has** to be the top level one by the rules of PMF)
https://eel.is/c++draft/module#private.frag-1
b) The contents of the PMF are not 'part of the module' reachable to importers of the parent module - so it seems that we only have to consider then when the TU is the current top level module interface.
https://eel.is/c++draft/module#reach-3
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1567-1568
+
+ return M->getPrimaryModuleInterfaceName() ==
+ Module::getPrimaryModuleInterfaceName(LangOpts.CurrentModule);
}
----------------
iains wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > iains wrote:
> > > > > ... of course, "correctness before optimisation", but this test function seems like it would benefit from some refactoring as suggested below.
> > > > >
> > > > > it seems a bit unfortunate that we are placing a string comparison in the "acceptable decl" lookup path, which might be made many times (the previous use of the string compare was in making the module decl - which only happens once in a TU).
> > > > >
> > > > > * Sema has visibility of the import graph of modules (via DirectModuleImports and Exports).
> > > > > * We also know what the module parse state is (FirstDecl, GMF etc)
> > > > > * If were are not parsing a module (i.e. LangOpts.CurrentModule.empty()) the we could return false early?
> > > > > * if ModuleScopes is empty we know we are not (yet) parsing a module
> > > > > * Once we are parsing a module then we can test ModuleScopes.back() to find the current module
> > > > >
> > > > > there are only two users of isInCurrentModule() - so maybe we refactor could make more use of the higher level state information - and / or cache information on the parents of partitions to avoid the complexity of parsing strings each time.
> > > > >
> > > > > what do you think?
> > > > Yeah, string comparison is relatively expensive and we should avoid that. I added a search cache here and I thought it should handle the most cases.
> > > >
> > > > For the solution you described, I am not sure if I understood correctly. It looks like a quick return path if we found we are not parsing a module? If yes, I think the current check could do similar works.
> > > That looks good.
> > >
> > > On the actual test itself:
> > >
> > > 1/
> > > we have the check of `(M->isGlobalModule() && !M->Parent)`
> > >
> > > So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
> > > if `M->isGlobalModule() && M->Parent` ?
> > > it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?
> > >
> > > 2/
> > > When we are actually parsing the PMF we have a similar case, no?
> > > (so that there is no Parent pointer set yet, and I think that means that we would need to examine ModuleScopes to find the Parent module?)
> > >
> > >
> > > 1/
> > > we have the check of (M->isGlobalModule() && !M->Parent)
> > >
> > > So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
> > > if M->isGlobalModule() && M->Parent ?
> > > it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?
> >
> > Good Catcha! Now in this revision, the function would return false for the case of `M->isGlobalModule() && M->Parent`. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name `isUsableModule` for it from the wording of `[module.global.frag]p1`. I tried to call it `isVisibleModule` but I found there is already one... I feel `isUsableModule` is not perfectly good... Any suggestion?
> >
> > > 2/
> > > When we are actually parsing the PMF we have a similar case, no?
> >
> > There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.
> > > 1/
> > > we have the check of (M->isGlobalModule() && !M->Parent)
> > >
> > > So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
> > > if M->isGlobalModule() && M->Parent ?
> > > it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?
> >
> > Good Catcha! Now in this revision, the function would return false for the case of `M->isGlobalModule() && M->Parent`. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name `isUsableModule` for it from the wording of `[module.global.frag]p1`. I tried to call it `isVisibleModule` but I found there is already one... I feel `isUsableModule` is not perfectly good... Any suggestion?
>
> actually that seems a reasonable name for now - I am beginning to feel that once we have things basically working - the whole module visibility stack could be refactored ...
>
> > > 2/
> > > When we are actually parsing the PMF we have a similar case, no?
> >
> > There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.
>
> Yes, but....
>
> a) When we are actually parsing the PMF, there will be no 'Parent' pointer set, so that we would have to find the parent some other way, right?
> (perhaps it is not too complex - since we should already have diagnosed a bad parse situation if we see a :private module line in the wrong place - so that all we need to be able to do is to locate the current module - that **has** to be the top level one by the rules of PMF)
>
> https://eel.is/c++draft/module#private.frag-1
>
> b) The contents of the PMF are not 'part of the module' reachable to importers of the parent module - so it seems that we only have to consider then when the TU is the current top level module interface.
>
> https://eel.is/c++draft/module#reach-3
>
>
> > 1/
> > we have the check of (M->isGlobalModule() && !M->Parent)
> >
> > So that answers "this decl is part of the global module", since that fires when we are parsing the GMF. What will we answer,
> > if M->isGlobalModule() && M->Parent ?
> > it seems that with the changes above we will now answer that the decl is part of the Parent - not part of the GM?
>
> Good Catcha! Now in this revision, the function would return false for the case of `M->isGlobalModule() && M->Parent`. Another point I found is that the name of the function is not quite right. Since global module fragment belongs to global module, it wouldn't be within the current module. So I chose a new name `isUsableModule` for it from the wording of `[module.global.frag]p1`. I tried to call it `isVisibleModule` but I found there is already one... I feel `isUsableModule` is not perfectly good... Any suggestion?
>
> > 2/
> > When we are actually parsing the PMF we have a similar case, no?
>
> There might not be a problem. Since PMF belongs to a named module technically. So the current implementation would handle it correctly.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123837/new/
https://reviews.llvm.org/D123837
More information about the cfe-commits
mailing list