iains marked 3 inline comments as done. iains added a comment. I will look at the rest of the comments once back in the office.
In D145965#4431929 <https://reviews.llvm.org/D145965#4431929>, @ChuanqiXu wrote: > In D145965#4431888 <https://reviews.llvm.org/D145965#4431888>, @iains wrote: > >> In D145965#4431846 <https://reviews.llvm.org/D145965#4431846>, @ChuanqiXu >> wrote: >> >>>> if we do not adjust the typo fixes, we will regress diagnostics. >>> >>> What the kind of diagnostics will be regressed? I mean, it looks weird to >>> me that we suggest typo fixes from hidden names. >> >> Because when we do typo checking, we relax the visibility conditions so that >> we can see some decl that might be hidden or misspelled - then we can say >> >> "you need to import module XX before using YY", >> >> or >> >> "did you mean ZZ" >> >> (I would be happy if we did not need to do this in this patch, but not sure >> how we can work around it). > > It is OK to see the misspelled decl. But it looks weird to me to see the > hidden decls. I think such regression should be called correction. It is a case that we have supported; the user puts in a use of a decl but forgets to import the module exporting it (I agree it is not _exactly_ a "typo" in terms of names, but the diagnostics counts it in the same way) ================ Comment at: clang/lib/Sema/SemaLookup.cpp:1782 assert(DeclModule && "hidden decl has no owning module"); + // If the owning module is visible, the decl is potentially acceptable. ---------------- ChuanqiXu wrote: > iains wrote: > > ChuanqiXu wrote: > > > It looks better to me if we can insert codes here > > I am not sure exactly what you mean by "insert codes"? > Oh, this is a little bit historical. I mean it looks better to do the main > job of the patch (correcting the visibility of internal linkage entities) if > possible. OK. I agree that is ideal; it remains to be seen if it is feasible. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:3912-3936 + if (Visible) { + if (!FM) + break; + assert (D->hasLinkage() && "an imported func with no linkage?"); + // Unless the module is a defining one for the + bool Ovr = true; + for (unsigned I = 0; I < CodeSynthesisContexts.size(); ++I) { ---------------- ChuanqiXu wrote: > iains wrote: > > ChuanqiXu wrote: > > > ChuanqiXu wrote: > > > > ChuanqiXu wrote: > > > > > ChuanqiXu wrote: > > > > > > What's the intention for the change? And why is the current > > > > > > behavior bad without this? > > > > > > What's the intention for the change? And why is the current > > > > > > behavior bad without this? > > > > > > > > > > > > > > Oh, I understand why I feel the code is not good since the decl with > > > > internal linkage or module linkage shouldn't be visible. So even if > > > > there are problems, we should handle them elsewhere. > > > Could we tried to address this? The change smells not so good. > > I am not sure what you mean here - I would like us to get this lookup stuff > > fixed for 17, so will be working on it when back in the office (traveling > > today) > > > > There is a different behaviour between cases where the entry is from an > > named module (but not the current one) and a different TU of the same named > > module. > I mean the we should put the similar things together as much as possible. It > looks that the codes are trying to correcting the visibilities returned from > `isVisible()`. But this sounds not good. Since it implies that the result > from `isVisible()` is not valid. > > > There is a different behaviour between cases where the entry is from an > > named module (but not the current one) and a different TU of the same named > > module. > > Maybe we can tried to solve this by adding other `isVisible` interfaces. > > > I would like us to get this lookup stuff fixed for 17, so will be working > > on it when back in the office (traveling today) > > Yeah, but we still have one month. And even if we didn't get it in time. > (Given the size of the patch) It is still OK to back port this before the > first week of September. So we can be more relaxed. > I mean the we should put the similar things together as much as possible. It > looks that the codes are trying to correcting the visibilities returned from > `isVisible()`. But this sounds not good. Since it implies that the result > from `isVisible()` is not valid. Yes, indeed, I agree - if we were starting from scratch, we would do all this in the lower-level 'isVisible' - but since that is used in different ways by the higher levels, it seemed quite tricky to change. > > There is a different behaviour between cases where the entry is from an > > named module (but not the current one) and a different TU of the same named > > module. > > Maybe we can tried to solve this by adding other `isVisible` interfaces. maybe that might work, I believe that when I looked before it was going to be quite complex, I will look again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145965/new/ https://reviews.llvm.org/D145965 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
