[PATCH] D123837: [C++20] [Modules] Judge isInCurrentModule currently

Wed Apr 20 03:41:31 PDT 2022

iains accepted this revision.
iains added a comment.
This revision is now accepted and ready to land.

thanks for multiple iterations!

I think maybe you are using a too old clang-format?
it seems that clang-format >= llvm-14  removes spaces around module partition colons  ... so `A : Part`==>`A:Part` and `: Impl` => `:Impl`.
IMO this is the correct formatting (since it follows the examples in the standard).

Anyway, this is most likely the reason for the clang format fails on the CI.

This now LGTM - but please fix the formatting in the test cases.

================
Comment at: clang/lib/Sema/SemaLookup.cpp:1567-1568
+
+  return M->getPrimaryModuleInterfaceName() ==
+         Module::getPrimaryModuleInterfaceName(LangOpts.CurrentModule);
 }
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > 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.
> > > 
> > > 
> > > I am beginning to feel that once we have things basically working - the whole module visibility stack could be refactored ...
> > 
> > Completely agreed.
> > 
> > > 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?
> > 
> > This is not right. When we are parsing the PMF, we already created its parent. So we could assign the Parent field. This is different from GMF.
> > 
> > >  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.
> > 
> > Yeah, I think the newest revision addresses the comment.
> It looks like there is anything missing?

> > 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?
> 
> This is not right. When we are parsing the PMF, we already created its parent. So we could assign the Parent field. This is different from GMF.

Indeed; I missed that the Parent is set on the creation for the PMF.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123837/new/

https://reviews.llvm.org/D123837