Re: [PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[David Blaikie via cfe-commits]

OK - go ahead and remove the tests then, if the functionality is now
covered by the buildbot+existing tests.

On Wed, Aug 9, 2017 at 12:04 PM Grang, Mandeep Singh 
wrote:

> > Ah, OK. I'm still curious about whether this results in a loss of test
> coverage. Without this test, would the bug it was testing still be caught
> by some test failure in at least one of the forward or reverse iteration
> modes?
>
> Sorry ... I missed that. Yes, the reverse iteration buildbot
> http://lab.llvm.org:8011/builders/reverse-iteration would test these.
> These tests were a stopgap solution anyway while the reverse builtbot was
> being setup.
>
> > I'll just do that (r310506 & r310508) - done! :)
> Thanks!
>
>
> --Mandeep
>
> On 8/9/2017 11:35 AM, David Blaikie wrote:
>
>
>
> On Wed, Aug 9, 2017 at 1:44 AM Grang, Mandeep Singh 
> wrote:
>
>> In D35043 I have removed the llvm tests which use -reverse-iterate. This
>> patch removes the clang tests.
>>
>
> Ah, OK. I'm still curious about whether this results in a loss of test
> coverage. Without this test, would the bug it was testing still be caught
> by some test failure in at least one of the forward or reverse iteration
> modes?
>
>
>> Should I post a later patch to change all "class PointerLikeTypeTraits"
>> to "struct PointerLikeTypeTraits"?
>>
>
> I'll just do that (r310506 & r310508) - done! :)
>
>
>>
>>
>> On 8/7/2017 2:50 PM, David Blaikie wrote:
>>
>>
>>
>> On Mon, Aug 7, 2017 at 12:08 PM Mandeep Singh Grang via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> mgrang added a comment.
>>>
>>> This patch does 3 things:
>>>
>>> 1. Get rid of the unit test objc-modern-metadata-visibility2.mm because
>>> this test check uses flag -reverse-iterate. This flag will be removed in
>>> https://reviews.llvm.org/D35043.
>>>
>>
>> Sure - please commit that separately (probably once D35043 is approved -
>> probably best to include that removal in D35043, or a separate patch that
>> /only/ removes the -reverse-iterate flag (& any tests that use it) as a
>> standalone change?).
>>
>> Does this test need a replacement? If this test is removed and the
>> underlying issue it was testing regresses, will one of the buildbots
>> (reverse or normal) catch the problem?
>>
>>
>>> 2. https://reviews.llvm.org/D35043 gets rid of the empty base
>>> definition for PointerLikeTypeTraits. This results in a compiler warning
>>> because PointerLikeTypeTrait has been defined as struct here while in the
>>> header it is a class. So I have changed struct to class.
>>>
>>
>> I'd probably go the other way - traits classes like this make more sense
>> as structs, I think - it only has public members & no implementation really
>> has any need for supporting private members.
>>
>>
>>> 3. Since I changed struct PointerLikeTypeTrait to class
>>> PointerLikeTypeTrait here, the member functions are no longer public now.
>>> This results in a compiler error. So I explicitly marked them as public
>>> here.
>>>
>>>
>>> https://reviews.llvm.org/D36386
>>>
>>>
>>>
>>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[Grang, Mandeep Singh via cfe-commits]

> Ah, OK. I'm still curious about whether this results in a loss of 
test coverage. Without this test, would the bug it was testing still be 
caught by some test failure in at least one of the forward or reverse 
iteration modes?


Sorry ... I missed that. Yes, the reverse iteration buildbot 
http://lab.llvm.org:8011/builders/reverse-iteration would test these. 
These tests were a stopgap solution anyway while the reverse builtbot 
was being setup.



> I'll just do that (r310506 & r310508) - done! :)
Thanks!


--Mandeep


On 8/9/2017 11:35 AM, David Blaikie wrote:



On Wed, Aug 9, 2017 at 1:44 AM Grang, Mandeep Singh 
> wrote:


In D35043 I have removed the llvm tests which use
-reverse-iterate. This patch removes the clang tests.


Ah, OK. I'm still curious about whether this results in a loss of test 
coverage. Without this test, would the bug it was testing still be 
caught by some test failure in at least one of the forward or reverse 
iteration modes?


Should I post a later patch to change all "class
PointerLikeTypeTraits" to "struct PointerLikeTypeTraits"?


I'll just do that (r310506 & r310508) - done! :)



On 8/7/2017 2:50 PM, David Blaikie wrote:



On Mon, Aug 7, 2017 at 12:08 PM Mandeep Singh Grang via
Phabricator > wrote:

mgrang added a comment.

This patch does 3 things:

1. Get rid of the unit test
objc-modern-metadata-visibility2.mm
 because this
test check uses flag -reverse-iterate. This flag will be
removed in https://reviews.llvm.org/D35043.


Sure - please commit that separately (probably once D35043 is
approved - probably best to include that removal in D35043, or a
separate patch that /only/ removes the -reverse-iterate flag (&
any tests that use it) as a standalone change?).

Does this test need a replacement? If this test is removed and
the underlying issue it was testing regresses, will one of the
buildbots (reverse or normal) catch the problem?

2. https://reviews.llvm.org/D35043 gets rid of the empty base
definition for PointerLikeTypeTraits. This results in a
compiler warning because PointerLikeTypeTrait has been
defined as struct here while in the header it is a class. So
I have changed struct to class.


I'd probably go the other way - traits classes like this make
more sense as structs, I think - it only has public members & no
implementation really has any need for supporting private members.

3. Since I changed struct PointerLikeTypeTrait to class
PointerLikeTypeTrait here, the member functions are no longer
public now. This results in a compiler error. So I explicitly
marked them as public here.


https://reviews.llvm.org/D36386







___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[David Blaikie via cfe-commits]

On Wed, Aug 9, 2017 at 1:44 AM Grang, Mandeep Singh 
wrote:

> In D35043 I have removed the llvm tests which use -reverse-iterate. This
> patch removes the clang tests.
>

Ah, OK. I'm still curious about whether this results in a loss of test
coverage. Without this test, would the bug it was testing still be caught
by some test failure in at least one of the forward or reverse iteration
modes?


> Should I post a later patch to change all "class PointerLikeTypeTraits" to
> "struct PointerLikeTypeTraits"?
>

I'll just do that (r310506 & r310508) - done! :)


>
>
> On 8/7/2017 2:50 PM, David Blaikie wrote:
>
>
>
> On Mon, Aug 7, 2017 at 12:08 PM Mandeep Singh Grang via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> mgrang added a comment.
>>
>> This patch does 3 things:
>>
>> 1. Get rid of the unit test objc-modern-metadata-visibility2.mm because
>> this test check uses flag -reverse-iterate. This flag will be removed in
>> https://reviews.llvm.org/D35043.
>>
>
> Sure - please commit that separately (probably once D35043 is approved -
> probably best to include that removal in D35043, or a separate patch that
> /only/ removes the -reverse-iterate flag (& any tests that use it) as a
> standalone change?).
>
> Does this test need a replacement? If this test is removed and the
> underlying issue it was testing regresses, will one of the buildbots
> (reverse or normal) catch the problem?
>
>
>> 2. https://reviews.llvm.org/D35043 gets rid of the empty base definition
>> for PointerLikeTypeTraits. This results in a compiler warning because
>> PointerLikeTypeTrait has been defined as struct here while in the header it
>> is a class. So I have changed struct to class.
>>
>
> I'd probably go the other way - traits classes like this make more sense
> as structs, I think - it only has public members & no implementation really
> has any need for supporting private members.
>
>
>> 3. Since I changed struct PointerLikeTypeTrait to class
>> PointerLikeTypeTrait here, the member functions are no longer public now.
>> This results in a compiler error. So I explicitly marked them as public
>> here.
>>
>>
>> https://reviews.llvm.org/D36386
>>
>>
>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[Grang, Mandeep Singh via cfe-commits]

In D35043 I have removed the llvm tests which use -reverse-iterate. This 
patch removes the clang tests.


Should I post a later patch to change all "class PointerLikeTypeTraits" 
to "struct PointerLikeTypeTraits"?


On 8/7/2017 2:50 PM, David Blaikie wrote:



On Mon, Aug 7, 2017 at 12:08 PM Mandeep Singh Grang via Phabricator 
> wrote:


mgrang added a comment.

This patch does 3 things:

1. Get rid of the unit test objc-modern-metadata-visibility2.mm
 because this test
check uses flag -reverse-iterate. This flag will be removed in
https://reviews.llvm.org/D35043.


Sure - please commit that separately (probably once D35043 is approved 
- probably best to include that removal in D35043, or a separate patch 
that /only/ removes the -reverse-iterate flag (& any tests that use 
it) as a standalone change?).


Does this test need a replacement? If this test is removed and the 
underlying issue it was testing regresses, will one of the buildbots 
(reverse or normal) catch the problem?


2. https://reviews.llvm.org/D35043 gets rid of the empty base
definition for PointerLikeTypeTraits. This results in a compiler
warning because PointerLikeTypeTrait has been defined as struct
here while in the header it is a class. So I have changed struct
to class.


I'd probably go the other way - traits classes like this make more 
sense as structs, I think - it only has public members & no 
implementation really has any need for supporting private members.


3. Since I changed struct PointerLikeTypeTrait to class
PointerLikeTypeTrait here, the member functions are no longer
public now. This results in a compiler error. So I explicitly
marked them as public here.


https://reviews.llvm.org/D36386





___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[David Blaikie via cfe-commits]

On Mon, Aug 7, 2017 at 12:08 PM Mandeep Singh Grang via Phabricator <
revi...@reviews.llvm.org> wrote:

> mgrang added a comment.
>
> This patch does 3 things:
>
> 1. Get rid of the unit test objc-modern-metadata-visibility2.mm because
> this test check uses flag -reverse-iterate. This flag will be removed in
> https://reviews.llvm.org/D35043.
>

Sure - please commit that separately (probably once D35043 is approved -
probably best to include that removal in D35043, or a separate patch that
/only/ removes the -reverse-iterate flag (& any tests that use it) as a
standalone change?).

Does this test need a replacement? If this test is removed and the
underlying issue it was testing regresses, will one of the buildbots
(reverse or normal) catch the problem?


> 2. https://reviews.llvm.org/D35043 gets rid of the empty base definition
> for PointerLikeTypeTraits. This results in a compiler warning because
> PointerLikeTypeTrait has been defined as struct here while in the header it
> is a class. So I have changed struct to class.
>

I'd probably go the other way - traits classes like this make more sense as
structs, I think - it only has public members & no implementation really
has any need for supporting private members.


> 3. Since I changed struct PointerLikeTypeTrait to class
> PointerLikeTypeTrait here, the member functions are no longer public now.
> This results in a compiler error. So I explicitly marked them as public
> here.
>
>
> https://reviews.llvm.org/D36386
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[Mandeep Singh Grang via Phabricator via cfe-commits]

mgrang added a comment.

This patch does 3 things:

1. Get rid of the unit test objc-modern-metadata-visibility2.mm because this 
test check uses flag -reverse-iterate. This flag will be removed in 
https://reviews.llvm.org/D35043.

2. https://reviews.llvm.org/D35043 gets rid of the empty base definition for 
PointerLikeTypeTraits. This results in a compiler warning because 
PointerLikeTypeTrait has been defined as struct here while in the header it is 
a class. So I have changed struct to class.

3. Since I changed struct PointerLikeTypeTrait to class PointerLikeTypeTrait 
here, the member functions are no longer public now. This results in a compiler 
error. So I explicitly marked them as public here.


https://reviews.llvm.org/D36386



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[David Blaikie via cfe-commits]

Not sure I understand the context for these changes - could you describe
the motivation(s) in more detail?

On Sun, Aug 6, 2017 at 10:39 PM Mandeep Singh Grang via Phabricator <
revi...@reviews.llvm.org> wrote:

> mgrang created this revision.
>
> This patch is in response to https://reviews.llvm.org/D35043 which
> removed -reverse-iterate flag
> and the base definition for PointerLikeTypeTrait.
>
>
> https://reviews.llvm.org/D36386
>
> Files:
>   include/clang/AST/ExternalASTSource.h
>   test/Rewriter/objc-modern-metadata-visibility2.mm
>
>
> Index: test/Rewriter/objc-modern-metadata-visibility2.mm
> ===
> --- test/Rewriter/objc-modern-metadata-visibility2.mm
> +++ /dev/null
> @@ -1,45 +0,0 @@
> -// REQUIRES: abi-breaking-checks
> -// NOTE: This test has been split from objc-modern-metadata-visibility.mm
> in
> -// order to test with -reverse-iterate as this flag is only present with
> -// ABI_BREAKING_CHECKS.
> -
> -// RUN: %clang_cc1 -E %s -o %t.mm -mllvm -reverse-iterate
> -// RUN: %clang_cc1 -x objective-c++ -fblocks -fms-extensions
> -rewrite-objc %t.mm -mllvm -reverse-iterate -o - | FileCheck %s
> -// rdar://11144048
> -
> -@class NSString;
> -
> -@interface NSObject {
> -Class isa;
> -}
> -@end
> -
> -@interface Sub : NSObject {
> -int subIvar;
> -NSString *nsstring;
> -@private
> -id PrivateIvar;
> -}
> -@end
> -
> -@implementation Sub
> -- (id) MyNSString { return subIvar ? PrivateIvar : nsstring; }
> -@end
> -
> -@interface NSString @end
> -@implementation NSString @end
> -
> -// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C"
> __declspec(dllimport) unsigned long OBJC_IVAR_$_Sub$subIvar;
> -// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" unsigned long
> OBJC_IVAR_$_Sub$PrivateIvar;
> -// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C"
> __declspec(dllimport) unsigned long OBJC_IVAR_$_Sub$nsstring;
> -// CHECK: #pragma warning(disable:4273)
> -// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C"
> __declspec(dllexport) unsigned long int OBJC_IVAR_$_Sub$subIvar
> -// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C"
> __declspec(dllexport) unsigned long int OBJC_IVAR_$_Sub$nsstring
> -// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" unsigned long
> int OBJC_IVAR_$_Sub$PrivateIvar
> -// CHECK: extern "C" __declspec(dllimport) struct _class_t
> OBJC_METACLASS_$_NSObject;
> -// CHECK: extern "C" __declspec(dllexport) struct _class_t
> OBJC_METACLASS_$_Sub
> -// CHECK: extern "C" __declspec(dllimport) struct _class_t
> OBJC_CLASS_$_NSObject;
> -// CHECK: extern "C" __declspec(dllexport) struct _class_t
> OBJC_CLASS_$_Sub
> -// CHECK: extern "C" __declspec(dllexport) struct _class_t
> OBJC_CLASS_$_NSString;
> -// CHECK: extern "C" __declspec(dllexport) struct _class_t
> OBJC_METACLASS_$_NSString
> -// CHECK: extern "C" __declspec(dllexport) struct _class_t
> OBJC_CLASS_$_NSString
> Index: include/clang/AST/ExternalASTSource.h
> ===
> --- include/clang/AST/ExternalASTSource.h
> +++ include/clang/AST/ExternalASTSource.h
> @@ -466,9 +466,10 @@
>  namespace llvm {
>  template   void (clang::ExternalASTSource::*Update)(Owner)>
> -struct PointerLikeTypeTraits<
> +class PointerLikeTypeTraits<
>  clang::LazyGenerationalUpdatePtr> {
>typedef clang::LazyGenerationalUpdatePtr Ptr;
> +public:
>static void *getAsVoidPointer(Ptr P) { return P.getOpaqueValue(); }
>static Ptr getFromVoidPointer(void *P) { return
> Ptr::getFromOpaqueValue(P); }
>enum {
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36386: [clang] Remove unit test which uses reverse-iterate and fix a PointerLikeTypeTrait specialization

[Mandeep Singh Grang via Phabricator via cfe-commits]

mgrang created this revision.

This patch is in response to https://reviews.llvm.org/D35043 which removed 
-reverse-iterate flag
and the base definition for PointerLikeTypeTrait.


https://reviews.llvm.org/D36386

Files:
  include/clang/AST/ExternalASTSource.h
  test/Rewriter/objc-modern-metadata-visibility2.mm


Index: test/Rewriter/objc-modern-metadata-visibility2.mm
===
--- test/Rewriter/objc-modern-metadata-visibility2.mm
+++ /dev/null
@@ -1,45 +0,0 @@
-// REQUIRES: abi-breaking-checks
-// NOTE: This test has been split from objc-modern-metadata-visibility.mm in
-// order to test with -reverse-iterate as this flag is only present with
-// ABI_BREAKING_CHECKS.
-
-// RUN: %clang_cc1 -E %s -o %t.mm -mllvm -reverse-iterate
-// RUN: %clang_cc1 -x objective-c++ -fblocks -fms-extensions -rewrite-objc 
%t.mm -mllvm -reverse-iterate -o - | FileCheck %s
-// rdar://11144048
-
-@class NSString;
-
-@interface NSObject {
-Class isa;
-}
-@end
-
-@interface Sub : NSObject {
-int subIvar;
-NSString *nsstring;
-@private
-id PrivateIvar;
-}
-@end
-
-@implementation Sub
-- (id) MyNSString { return subIvar ? PrivateIvar : nsstring; }
-@end
-
-@interface NSString @end
-@implementation NSString @end
-
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" 
__declspec(dllimport) unsigned long OBJC_IVAR_$_Sub$subIvar;
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" unsigned long 
OBJC_IVAR_$_Sub$PrivateIvar;
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" 
__declspec(dllimport) unsigned long OBJC_IVAR_$_Sub$nsstring;
-// CHECK: #pragma warning(disable:4273)
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" 
__declspec(dllexport) unsigned long int OBJC_IVAR_$_Sub$subIvar
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" 
__declspec(dllexport) unsigned long int OBJC_IVAR_$_Sub$nsstring
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" unsigned long int 
OBJC_IVAR_$_Sub$PrivateIvar
-// CHECK: extern "C" __declspec(dllimport) struct _class_t 
OBJC_METACLASS_$_NSObject;
-// CHECK: extern "C" __declspec(dllexport) struct _class_t OBJC_METACLASS_$_Sub
-// CHECK: extern "C" __declspec(dllimport) struct _class_t 
OBJC_CLASS_$_NSObject;
-// CHECK: extern "C" __declspec(dllexport) struct _class_t OBJC_CLASS_$_Sub
-// CHECK: extern "C" __declspec(dllexport) struct _class_t 
OBJC_CLASS_$_NSString;
-// CHECK: extern "C" __declspec(dllexport) struct _class_t 
OBJC_METACLASS_$_NSString
-// CHECK: extern "C" __declspec(dllexport) struct _class_t 
OBJC_CLASS_$_NSString
Index: include/clang/AST/ExternalASTSource.h
===
--- include/clang/AST/ExternalASTSource.h
+++ include/clang/AST/ExternalASTSource.h
@@ -466,9 +466,10 @@
 namespace llvm {
 template
-struct PointerLikeTypeTraits<
+class PointerLikeTypeTraits<
 clang::LazyGenerationalUpdatePtr> {
   typedef clang::LazyGenerationalUpdatePtr Ptr;
+public:
   static void *getAsVoidPointer(Ptr P) { return P.getOpaqueValue(); }
   static Ptr getFromVoidPointer(void *P) { return Ptr::getFromOpaqueValue(P); }
   enum {


Index: test/Rewriter/objc-modern-metadata-visibility2.mm
===
--- test/Rewriter/objc-modern-metadata-visibility2.mm
+++ /dev/null
@@ -1,45 +0,0 @@
-// REQUIRES: abi-breaking-checks
-// NOTE: This test has been split from objc-modern-metadata-visibility.mm in
-// order to test with -reverse-iterate as this flag is only present with
-// ABI_BREAKING_CHECKS.
-
-// RUN: %clang_cc1 -E %s -o %t.mm -mllvm -reverse-iterate
-// RUN: %clang_cc1 -x objective-c++ -fblocks -fms-extensions -rewrite-objc %t.mm -mllvm -reverse-iterate -o - | FileCheck %s
-// rdar://11144048
-
-@class NSString;
-
-@interface NSObject {
-Class isa;
-}
-@end
-
-@interface Sub : NSObject {
-int subIvar;
-NSString *nsstring;
-@private
-id PrivateIvar;
-}
-@end
-
-@implementation Sub
-- (id) MyNSString { return subIvar ? PrivateIvar : nsstring; }
-@end
-
-@interface NSString @end
-@implementation NSString @end
-
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" __declspec(dllimport) unsigned long OBJC_IVAR_$_Sub$subIvar;
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" unsigned long OBJC_IVAR_$_Sub$PrivateIvar;
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" __declspec(dllimport) unsigned long OBJC_IVAR_$_Sub$nsstring;
-// CHECK: #pragma warning(disable:4273)
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" __declspec(dllexport) unsigned long int OBJC_IVAR_$_Sub$subIvar
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" __declspec(dllexport) unsigned long int OBJC_IVAR_$_Sub$nsstring
-// CHECK: __declspec(allocate(".objc_ivar$B")) extern "C" unsigned long int OBJC_IVAR_$_Sub$PrivateIvar
-// CHECK: extern "C" __declspec(dllimport) struct _class_t