[PATCH] D19468: Disallow duplication of imported entities (improved implementation)

Wed Apr 27 11:23:00 PDT 2016

On Wed, Apr 27, 2016 at 12:46 AM, Aboud, Amjad <amjad.aboud at intel.com>
wrote:

> I do not know if this is the only case where this happens, but consider
> the following:
>
>
>
> *TestIM.cpp*
>
> #include "TestIM.h"
>
> #include "TestIM.h"
>
>
>
> *TestIM.h*
>
> namespace X {
>
>   typedef int MyINT_t;
>
> }
>
>
>
> namespace Y {
>
>   using X::MyINT_t;
>
> }
>
>
>
> !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1,
> producer: "clang version 3.8.0 (trunk 257325)", isOptimized: false,
> runtimeVersion: 0, emissionKind: 1, enums: !2, imports: !3)
>
> !3 = !{!4, !4}
>
> !4 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !5,
> entity: !7, line: 6)
>
>
>
> Currently we just emit the imported module twice, but once I added my fix
> to Lexical scope, I had an assumption that “imports” list is unique.
>
> I could fix that in backend of course, but I think there is no added value
> to have the duplication in the IR.
>
>
>
> Do you agree?
>

I'm not sure I'm terribly fussed about the duplication here.

If it shows up a lot or is otherwise really problematic, sure, it may be
worth a fix of some kind.

Ultimately the way using decls are handled is sub-optimal (& I implemented
them this way, so I'm not pointing fingers or anything) because even unused
ones end up emitted which is kind of a pain for debug info size,
readability, etc. At some point we'd want to try to reverse the association
(this has been speculated about on the threads regarding the subprogram
link reversals, global variable link reversal speculation, etc) so that
unused ones go away. That way duplicates wouldn't be much of an issue,
they'd just disappear that way.

But yes, it doesn't seem sufficiently compelling to include these or emit
them or require a frontend to deduplicate them as they are distinct
entities at that level. I just didn't want to add another level of set
deduplication if there was an obvious mechanism to avoid creating
redundancy in the first place and it seems there isn't. Thanks for bearing
with me.

- Dave

>
>
> Regards,
>
> Amjad
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* Tuesday, April 26, 2016 21:40
> *To:* Aboud, Amjad <amjad.aboud at intel.com>
> *Cc:* reviews+D19468+public+d141c7248d054ca3 at reviews.llvm.org; Duncan P.
> N. Exon Smith <dexonsmith at apple.com>; Adrian Prantl <aprantl at apple.com>;
> llvm-commits <llvm-commits at lists.llvm.org>
>
> *Subject:* Re: [PATCH] D19468: Disallow duplication of imported entities
> (improved implementation)
>
>
>
>
>
>
>
> On Tue, Apr 26, 2016 at 1:01 AM, Aboud, Amjad <amjad.aboud at intel.com>
> wrote:
>
> You cannot simply skip a redeclaration unless it is declared at same
> location.
>
>
>
> For example:
>
>
>
> namespace X { int a; }
>
> // some other code
>
> namespace X { int b; }
>
>
>
> You cannot eliminate the second namespace, can you?
>
>
>
> It's not the declarations of the namespace that I'm referring to - it's
> the using declarations
>
>
>
>
>
> void foo () { using X:a; }
>
> void bar () { using X:a; }
>
>
>
> Also here, you cannot eliminate the second using-decl can you?
>
>
>
> These wouldn't be redeclarations of each other, I don't think. They
> represent different decls in each scope (that resolve to the same entity,
> but are themselves distinct declarations).
>
> But no, it seems like two identical using declarations in the same scope
> aren't considered to be redeclarations of each other, so my suggestion
> doesn't apply in any case.
>
> Could you remind me why this comes up? If the user wrote two of the same
> using decls, it doesn't seem like the worst thing if we produce equally
> redundant output. If I recall correctly, we had some correctness
> problem/some case where we were emitting two using decls even though the
> user only wrote one?
>
>
>
>
>
> Looking into the code, it is not simple to introduce these checks, as
> there is no one place that will affect all Decls, and we will need to
> handle each Decl separately.
>
> Can you think on a place where we can add this in AST?
>
>
>
> Thanks,
>
> Amjad
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* Tuesday, April 26, 2016 03:12
> *To:* reviews+D19468+public+d141c7248d054ca3 at reviews.llvm.org; Aboud,
> Amjad <amjad.aboud at intel.com>
> *Cc:* Duncan P. N. Exon Smith <dexonsmith at apple.com>; Adrian Prantl <
> aprantl at apple.com>; llvm-commits <llvm-commits at lists.llvm.org>
> *Subject:* Re: [PATCH] D19468: Disallow duplication of imported entities
> (improved implementation)
>
>
>
>
>
>
>
> On Mon, Apr 25, 2016 at 2:25 PM, Amjad Aboud via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> aaboud added a comment.
>
> I am not much familiar with the AST part, but I assume there is reuse of
> previous generated DCL, even if it has same data.
>
>
> What if we filter by cases where the decl == the canonical decl, and skip
> any decl that's a redeclaration?
>
>
>
> Here is a small example that leads into duplication in AST DCLs.
> You may run the following command line:
>
>   clang -cc1 -ast-dump -o - -O0 TestIM.cpp
>
> TestIM.cpp
> ----------
>
>   #include "TestIM.h"
>   #include "TestIM.h"
>
>
>
> TestIM.h
> --------
>
>   namespace X {
>     typedef int MyINT_t;
>   }
>
>   namespace Y {
>     using X::MyINT_t;
>
>   }
>
>
> http://reviews.llvm.org/D19468
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>
>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160427/bc6d481a/attachment.html>