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

[Aboud, Amjad via llvm-commits]

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?

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<mailto: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<mailto:dblaikie at gmail.com>]
Sent: Tuesday, April 26, 2016 03:12
To: reviews+D19468+public+d141c7248d054ca3 at reviews.llvm.org<mailto:reviews%2BD19468%2Bpublic%2Bd141c7248d054ca3 at reviews.llvm.org>; Aboud, Amjad <amjad.aboud at intel.com<mailto:amjad.aboud at intel.com>>
Cc: Duncan P. N. Exon Smith <dexonsmith at apple.com<mailto:dexonsmith at apple.com>>; Adrian Prantl <aprantl at apple.com<mailto:aprantl at apple.com>>; llvm-commits <llvm-commits at lists.llvm.org<mailto: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<mailto: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<mailto: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/401017d5/attachment.html>