[PATCH] D78660: [SemaObjC] Add a warning for dictionary literals with duplicate keys

[Ben D. Jones via Phabricator via cfe-commits]

bendjones accepted this revision.
bendjones marked an inline comment as done.
bendjones added a comment.
This revision is now accepted and ready to land.

This addresses what we had an issue with on the tin. I’ll defer to @rjmccall 
for the specifics at the clang level.




Comment at: clang/lib/Sema/SemaExprObjC.cpp:948
+checkOneKey(IntegralKeys, Result.Val.getInt(), Loc);
+  }
+}

rjmccall wrote:
> Does `EvaluateAsInt` really just fail cleanly if the argument doesn't have 
> integral type?
I thought it fails if a type can’t be “some how” converted to an integral type. 
 The loose quotes are key here. 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78660/new/

https://reviews.llvm.org/D78660



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

[PATCH] D78066: [SemaObjC] Forbid storing an unboxed integer literal in an NSNumber

[Ben D. Jones via Phabricator via cfe-commits]

bendjones added a comment.

Seems reasonable to me. Thanks @erik.pilkington!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78066/new/

https://reviews.llvm.org/D78066



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

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[Ben D. Jones via Phabricator via cfe-commits]

bendjones added a comment.

In D70284#1978948 , @dexonsmith wrote:

> In D70284#1752893 , @bendjones wrote:
>
> > In D70284#1752811 , @dexonsmith 
> > wrote:
> >
> > > For some reason this revision is locked down and I'm not allowed to 
> > > "edit" it, which includes adding inline review comments.  Can you add me 
> > > as a reviewer?
> >
> >
> > Thought I did.
>
>
> I'm still not able to edit this, so I cannot "accept revision".  I'm not sure 
> what's going wrong but this revision object seems corrupted.


Hmm ok should I redo this completely then? Not sure what is up but it shows you 
as a reviewer hmm...

>>> The two comments:
>>> 
>>> - Please add a period at the end of the sentence in the comment.
>> 
>> Will do.
> 
> The comment actually looks liable to bitrot, since IIUC it's talking about 
> callers which can change over time.
> 
> LGTM if you remove it.

Ok will do...

> 
> 
>>> - Can you give more context about what `objc_arc_inert` is doing, and why 
>>> it's necessary now that `no_dead_strip` is gone?
>> 
>> The objc_arc_inert was added to other similar things so in the spirt of 
>> making things match I added it here. I can keep it simple though.
> 
> Okay, SGTM.  There are no ARC operations, and this will allow certain 
> optimizations.  I suggest documenting why it's safe in the commit message.

So just stating that I'm keeping things in sync with other areas?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70284/new/

https://reviews.llvm.org/D70284



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

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[Ben D. Jones via Phabricator via cfe-commits]

bendjones added a comment.

In D70284#1752806 , @bendjones wrote:

> Any additional thoughts @dexonsmith @erik.pilkington @ahatanak?


@dexonsmith @erik.pilkington @ahatanak does this look good to go?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70284/new/

https://reviews.llvm.org/D70284



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

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[Ben D. Jones via Phabricator via cfe-commits]

bendjones added a comment.

In D70284#1752811 , @dexonsmith wrote:

> For some reason this revision is locked down and I'm not allowed to "edit" 
> it, which includes adding inline review comments.  Can you add me as a 
> reviewer?


Thought I did.

> The two comments:
> 
> - Please add a period at the end of the sentence in the comment.

Will do.

> - Can you give more context about what `objc_arc_inert` is doing, and why 
> it's necessary now that `no_dead_strip` is gone?

The objc_arc_inert was added to other similar things so in the spirt of making 
things match I added it here. I can keep it simple though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70284/new/

https://reviews.llvm.org/D70284



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

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[Ben D. Jones via Phabricator via cfe-commits]

bendjones added a comment.

Any additional thoughts @dexonsmith @erik.pilkington @ahatanak?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70284/new/

https://reviews.llvm.org/D70284



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

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[Ben D. Jones via Phabricator via cfe-commits]

bendjones updated this revision to Diff 229441.
bendjones added a comment.

Added `"objc_arc_inert"` and updated 
`clang/test/CodeGenObjC/ns-constant-strings.m` tests to make sure the section 
is emitted as we want.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70284/new/

https://reviews.llvm.org/D70284

Files:
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/test/CodeGenObjC/ns-constant-strings.m


Index: clang/test/CodeGenObjC/ns-constant-strings.m
===
--- clang/test/CodeGenObjC/ns-constant-strings.m
+++ clang/test/CodeGenObjC/ns-constant-strings.m
@@ -33,7 +33,14 @@
 // CHECK-NONFRAGILE: @"OBJC_CLASS_$_NSConstantString" = external global
 
 // CHECK-FRAGILE: @.str = private unnamed_addr constant [6 x i8] c"MyApp\00"
+// CHECK-FRAGILE: @_unnamed_nsstring_ = private constant 
%struct.__builtin_NSString { i32* getelementptr inbounds ([0 x i32], [0 x i32]* 
@_NSConstantStringClassReference, i32 0, i32 0), i8* getelementptr inbounds ([6 
x i8], [6 x i8]* @.str, i32 0, i32 0), i32 5 }, section 
"__OBJC,__cstring_object,regular"
 // CHECK-FRAGILE: @.str.1 = private unnamed_addr constant [7 x i8] c"MyApp1\00"
+// CHECK-FRAGILE: @_unnamed_nsstring_.2 = private constant 
%struct.__builtin_NSString { i32* getelementptr inbounds ([0 x i32], [0 x i32]* 
@_NSConstantStringClassReference, i32 0, i32 0), i8* getelementptr inbounds ([7 
x i8], [7 x i8]* @.str.1, i32 0, i32 0), i32 6 }, section 
"__OBJC,__cstring_object,regular"
 
 // CHECK-NONFRAGILE: @.str = private unnamed_addr constant [6 x i8] c"MyApp\00"
+// CHECK-NONFRAGILE: @_unnamed_nsstring_ = private constant 
%struct.__builtin_NSString { i32* bitcast (%struct._class_t* 
@"OBJC_CLASS_$_NSConstantString" to i32*), i8* getelementptr inbounds ([6 x 
i8], [6 x i8]* @.str, i32 0, i32 0), i32 5 }, section 
"__DATA,__objc_stringobj,regular"
 // CHECK-NONFRAGILE: @.str.1 = private unnamed_addr constant [7 x i8] 
c"MyApp1\00"
+// CHECK-NONFRAGILE: @_unnamed_nsstring_.2 = private constant 
%struct.__builtin_NSString { i32* bitcast (%struct._class_t* 
@"OBJC_CLASS_$_NSConstantString" to i32*), i8* getelementptr inbounds ([7 x 
i8], [7 x i8]* @.str.1, i32 0, i32 0), i32 6 }, section 
"__DATA,__objc_stringobj,regular"
+
+// CHECK-FRAGILE: attributes #0 = { "objc_arc_inert" }
+// CHECK-NONFRAGILE: attributes #0 = { "objc_arc_inert" }
\ No newline at end of file
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -1978,6 +1978,7 @@
   return V;
 }
 
+// This is only used when `-fno-constant-cfstrings` is given
 ConstantAddress
 CGObjCCommonMac::GenerateConstantNSString(const StringLiteral *Literal) {
   unsigned StringLength = 0;
@@ -2029,13 +2030,12 @@
   GV = Fields.finishAndCreateGlobal("_unnamed_nsstring_", Alignment,
 /*constant*/ true,
 llvm::GlobalVariable::PrivateLinkage);
-  const char *NSStringSection = 
"__OBJC,__cstring_object,regular,no_dead_strip";
-  const char *NSStringNonFragileABISection =
-  "__DATA,__objc_stringobj,regular,no_dead_strip";
-  // FIXME. Fix section.
+  const char *NSStringSection = "__OBJC,__cstring_object,regular";
+  const char *NSStringNonFragileABISection = "__DATA,__objc_stringobj,regular";
   GV->setSection(CGM.getLangOpts().ObjCRuntime.isNonFragile()
  ? NSStringNonFragileABISection
  : NSStringSection);
+  GV->addAttribute("objc_arc_inert");
   Entry.second = GV;
 
   return ConstantAddress(GV, Alignment);


Index: clang/test/CodeGenObjC/ns-constant-strings.m
===
--- clang/test/CodeGenObjC/ns-constant-strings.m
+++ clang/test/CodeGenObjC/ns-constant-strings.m
@@ -33,7 +33,14 @@
 // CHECK-NONFRAGILE: @"OBJC_CLASS_$_NSConstantString" = external global
 
 // CHECK-FRAGILE: @.str = private unnamed_addr constant [6 x i8] c"MyApp\00"
+// CHECK-FRAGILE: @_unnamed_nsstring_ = private constant %struct.__builtin_NSString { i32* getelementptr inbounds ([0 x i32], [0 x i32]* @_NSConstantStringClassReference, i32 0, i32 0), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @.str, i32 0, i32 0), i32 5 }, section "__OBJC,__cstring_object,regular"
 // CHECK-FRAGILE: @.str.1 = private unnamed_addr constant [7 x i8] c"MyApp1\00"
+// CHECK-FRAGILE: @_unnamed_nsstring_.2 = private constant %struct.__builtin_NSString { i32* getelementptr inbounds ([0 x i32], [0 x i32]* @_NSConstantStringClassReference, i32 0, i32 0), i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str.1, i32 0, i32 0), i32 6 }, section "__OBJC,__cstring_object,regular"
 
 // CHECK-NONFRAGILE: @.str = private unnamed_addr constant [6 x i8] c"MyApp\00"
+// CHECK-NONFRAGILE: @_unnamed_nsstring_ = private constant %struct.__builtin_NSString { i32* bitcast (%struct._class_t* @"OBJC_

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[Ben D. Jones via Phabricator via cfe-commits]

bendjones added a comment.

In D70284#1746853 , @dexonsmith wrote:

> The code looks right, but can you add a test for this in CodeGenObjC?  Also, 
> when you re-upload the patch with the tests, please use `-U999` to 
> provide full context in the diff.


Will do


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70284/new/

https://reviews.llvm.org/D70284



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

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[Ben D. Jones via Phabricator via cfe-commits]

bendjones created this revision.
bendjones added reviewers: erik.pilkington, ahatanak.
bendjones created this object with edit policy "Administrators".
bendjones added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.

Part of rdar://56643852 that makes all possible constant string code paths emit 
without `no_dead_strip` so that dead stripping is allowed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70284

Files:
  clang/lib/CodeGen/CGObjCMac.cpp


Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -1978,6 +1978,7 @@
   return V;
 }
 
+// This is only used when `-fno-constant-cfstrings` is given
 ConstantAddress
 CGObjCCommonMac::GenerateConstantNSString(const StringLiteral *Literal) {
   unsigned StringLength = 0;
@@ -2029,10 +2030,8 @@
   GV = Fields.finishAndCreateGlobal("_unnamed_nsstring_", Alignment,
 /*constant*/ true,
 llvm::GlobalVariable::PrivateLinkage);
-  const char *NSStringSection = 
"__OBJC,__cstring_object,regular,no_dead_strip";
-  const char *NSStringNonFragileABISection =
-  "__DATA,__objc_stringobj,regular,no_dead_strip";
-  // FIXME. Fix section.
+  const char *NSStringSection = "__OBJC,__cstring_object,regular";
+  const char *NSStringNonFragileABISection = "__DATA,__objc_stringobj,regular";
   GV->setSection(CGM.getLangOpts().ObjCRuntime.isNonFragile()
  ? NSStringNonFragileABISection
  : NSStringSection);


Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -1978,6 +1978,7 @@
   return V;
 }
 
+// This is only used when `-fno-constant-cfstrings` is given
 ConstantAddress
 CGObjCCommonMac::GenerateConstantNSString(const StringLiteral *Literal) {
   unsigned StringLength = 0;
@@ -2029,10 +2030,8 @@
   GV = Fields.finishAndCreateGlobal("_unnamed_nsstring_", Alignment,
 /*constant*/ true,
 llvm::GlobalVariable::PrivateLinkage);
-  const char *NSStringSection = "__OBJC,__cstring_object,regular,no_dead_strip";
-  const char *NSStringNonFragileABISection =
-  "__DATA,__objc_stringobj,regular,no_dead_strip";
-  // FIXME. Fix section.
+  const char *NSStringSection = "__OBJC,__cstring_object,regular";
+  const char *NSStringNonFragileABISection = "__DATA,__objc_stringobj,regular";
   GV->setSection(CGM.getLangOpts().ObjCRuntime.isNonFragile()
  ? NSStringNonFragileABISection
  : NSStringSection);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits