================ @@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider inconsistent macro definit Currently Clang would accept the above example. But it may produce surprising results if the debugging code depends on consistent use of ``NDEBUG`` also in other translation units. +Definitions consistency +^^^^^^^^^^^^^^^^^^^^^^^ + +The C++ language defines that same declarations in different translation units should have +the same definition, as known as ODR (One Definition Rule). Prior to modules, the translation +units don't dependent on each other and the compiler itself don't and can't perform a strong +ODR violation check. Sometimes it is the linker does some jobs related to ODR, where the +higher level semantics are missing. With the introduction of modules, now the compiler have +the chance to perform ODR violations with language semantics across translation units. + +However, in the practice we found the existing ODR checking mechanism may be too aggressive. +In the many issue reports about ODR violation diagnostics, most of them are false positive +ODR violations and the true positive ODR violations are rarely reported. Also MSVC don't +perform ODR check for declarations in the global module fragment. ---------------- mizvekov wrote:
> Great insight. I feel this is meaningful. Maybe we need to reach out WG21 to > confirm this. Can you access the mailing list of WG21? I am not a member yet, but I know a few people in there. > Sorry. I still don't understand why it is related to the patch itself. So a little background: The issue #78850 that was referenced in the original commit, which seems to be the main motivator for that change, was reported as two parts: 1) A real-world-like repro where the user is just including different libstdc++ headers. 2) The reduced repro, where there is the real ODR violation we were just arguing about. I have reason to believe, based on similarity to clang bug I am currently working on, that 2) is unrelated to 1). Based on the commonality between reproducers of #78850 and the bug I am working on. They both involve UsingShadowDecls, the merging of which is broken, as I will show on an upcoming patch. If you look at this from another angle, this reduction was performed by the user / reporter, and reducing a false-positive ODR violation repro is hard, because it's so easy you would introduce a real ODR-violation during a reduction step. So it's reasonable to double-check / do some due-dilligence here. So I feel we are taking a drastic step here based on potentially incorrect evidence. If we can confirm what I am saying, then don't you agree we would be losing the whole casus belli against the GMF ODR checker, and we could go back to square one, revert the whole thing, and take a better look at this? Even if that is not the only issue, my perception is that other supposed ODR violations could be simple issues like this. And even if there are ODR violations in shipped system headers, couldn't we: 1) Provide a more targeted workaround rather than disabling the whole thing. 2) Stand our ground, since the whole C++20 modules thing is new, we can expect users to be using more recent / fixed system headers. > > But the explanation from MSVC developer moves me. Since the entities in the > GMF are basically from headers. Then it wouldn't be a regression in some > level. I have never seen MSVC source code, so I don't know how relevant it would be for comparison, but I think clang has enough differences in how sema is applied to textual source vs PCM, that this would give me pause and I wouldn't be so sure. On the other hand, this does not seem to be a regression, since the whole C++20 modules thing is just coming up and we are talking about a new feature and new users. > > Also I feel your concern here is about deeper other problems. (e.g., what is > ODR violation?) And I want to backport the series patches to 18.x. So I am > wondering if we can approve this patch and I can start to backport it. Then > we can discuss your concern in other places. See above. Would you mind waiting a little for my patch, for us to double check we are taking this step with solid footing? https://github.com/llvm/llvm-project/pull/79959 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
