Re: r288269 - Finish adopting ConstantInitBuilder in CGObjCGNU. NFC.

[John McCall via cfe-commits]

> On Nov 30, 2016, at 3:27 PM, Reid Kleckner  wrote:
> 
> So, I always push for leading upper case variable names. Over time we've 
> gotten to the point where the majority of the code that I read in clang 
> follows this convention. You seem to be pushing the other direction. What do 
> you think we should be doing?

IRGen has always been inconsistent about this, and my goal in IRGen is to make 
it consistently use leading lowercase.  I have always been up-front about this 
goal; the pervasive LLVM style where literally everything is uppercased is, as 
you say, dumb and unreadable, as well as creating massive spurious conflicts 
between types and variables.  IRGen already has enough readability problems 
from working with different layers with a ton of similarly-named types and 
concepts (and trust me, it's somehow worse in Swift's IRGen), as well as the 
monotonous verbosity of having a thing and lowering it to a different thing, 
over and over and over; it really does not need any extra disadvantages.  So 
while I won't reformat existing code for no reason, when I do find myself 
rewriting it substantially, I usually try to make that function use lowercase, 
at least for spelled-out names.

If someone wrote a tool to mass-reformat IRGen to use consistent uppercase, I 
would complain — we'd be enforcing a convention that it seems everyone who 
contributes to LLVM has independently decided is bad but which nobody has the 
energy to fight for (but who would they be fighting?) — but in the end I'd 
apply it and endeavor to follow the rule consistently thenceforth.  But since 
significant amounts of IRGen do not use that convention already, I am not going 
to follow it.

John.

> 
> I'll admit the LLVM naming conventions are dumb, but I just don't care 
> anymore, and would rather have it all look the same.
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
>  
> 
> 
> I only mention it because you're modifying code that was clearly in the 
> leading uppercase camp to leading lower case, so you weren't fitting into an 
> existing style common to some local source file.
> 
> On Wed, Nov 30, 2016 at 12:19 PM, John McCall via cfe-commits 
> > wrote:
> Author: rjmccall
> Date: Wed Nov 30 14:19:46 2016
> New Revision: 288269
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=288269=rev 
> 
> Log:
> Finish adopting ConstantInitBuilder in CGObjCGNU.  NFC.
> 
> Modified:
> cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
> cfe/trunk/lib/CodeGen/ConstantBuilder.h
> 
> Modified: cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCGNU.cpp?rev=288269=288268=288269=diff
>  
> 
> ==
> --- cfe/trunk/lib/CodeGen/CGObjCGNU.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGObjCGNU.cpp Wed Nov 30 14:19:46 2016
> @@ -164,9 +164,8 @@ protected:
>/// Helper function that generates a constant string and returns a pointer 
> to
>/// the start of the string.  The result of this function can be used 
> anywhere
>/// where the C code specifies const char*.
> -  llvm::Constant *MakeConstantString(const std::string ,
> - const std::string ="") {
> -ConstantAddress Array = CGM.GetAddrOfConstantCString(Str, Name.c_str());
> +  llvm::Constant *MakeConstantString(StringRef Str, const char *Name = "") {
> +ConstantAddress Array = CGM.GetAddrOfConstantCString(Str, Name);
>  return llvm::ConstantExpr::getGetElementPtr(Array.getElementType(),
>  Array.getPointer(), Zeros);
>}
> @@ -1530,23 +1529,24 @@ GenerateMethodList(StringRef ClassName,
>MethodList.addInt(Int32Ty, MethodTypes.size());
> 
>// Get the method structure type.
> -  llvm::StructType *ObjCMethodTy = llvm::StructType::get(
> -PtrToInt8Ty, // Really a selector, but the runtime creates it us.
> -PtrToInt8Ty, // Method types
> -IMPTy, //Method pointer
> -nullptr);
> +  llvm::StructType *ObjCMethodTy =
> +llvm::StructType::get(CGM.getLLVMContext(), {
> +  PtrToInt8Ty, // Really a selector, but the runtime creates it us.
> +  PtrToInt8Ty, // Method types
> +  IMPTy// Method pointer
> +});
>auto Methods = MethodList.beginArray();
>for (unsigned int i = 0, e = MethodTypes.size(); i < e; ++i) {
> -llvm::Constant *Method =
> +llvm::Constant *FnPtr =
>TheModule.getFunction(SymbolNameForMethod(ClassName, CategoryName,
>  MethodSels[i],
>   

Re: r288269 - Finish adopting ConstantInitBuilder in CGObjCGNU. NFC.

[Reid Kleckner via cfe-commits]

So, I always push for leading upper case variable names. Over time we've
gotten to the point where the majority of the code that I read in clang
follows this convention. You seem to be pushing the other direction. What
do you think we should be doing?

I'll admit the LLVM naming conventions are dumb, but I just don't care
anymore, and would rather have it all look the same.
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

I only mention it because you're modifying code that was clearly in the
leading uppercase camp to leading lower case, so you weren't fitting into
an existing style common to some local source file.

On Wed, Nov 30, 2016 at 12:19 PM, John McCall via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rjmccall
> Date: Wed Nov 30 14:19:46 2016
> New Revision: 288269
>
> URL: http://llvm.org/viewvc/llvm-project?rev=288269=rev
> Log:
> Finish adopting ConstantInitBuilder in CGObjCGNU.  NFC.
>
> Modified:
> cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
> cfe/trunk/lib/CodeGen/ConstantBuilder.h
>
> Modified: cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/
> CGObjCGNU.cpp?rev=288269=288268=288269=diff
> 
> ==
> --- cfe/trunk/lib/CodeGen/CGObjCGNU.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGObjCGNU.cpp Wed Nov 30 14:19:46 2016
> @@ -164,9 +164,8 @@ protected:
>/// Helper function that generates a constant string and returns a
> pointer to
>/// the start of the string.  The result of this function can be used
> anywhere
>/// where the C code specifies const char*.
> -  llvm::Constant *MakeConstantString(const std::string ,
> - const std::string ="") {
> -ConstantAddress Array = CGM.GetAddrOfConstantCString(Str,
> Name.c_str());
> +  llvm::Constant *MakeConstantString(StringRef Str, const char *Name =
> "") {
> +ConstantAddress Array = CGM.GetAddrOfConstantCString(Str, Name);
>  return llvm::ConstantExpr::getGetElementPtr(Array.getElementType(),
>  Array.getPointer(),
> Zeros);
>}
> @@ -1530,23 +1529,24 @@ GenerateMethodList(StringRef ClassName,
>MethodList.addInt(Int32Ty, MethodTypes.size());
>
>// Get the method structure type.
> -  llvm::StructType *ObjCMethodTy = llvm::StructType::get(
> -PtrToInt8Ty, // Really a selector, but the runtime creates it us.
> -PtrToInt8Ty, // Method types
> -IMPTy, //Method pointer
> -nullptr);
> +  llvm::StructType *ObjCMethodTy =
> +llvm::StructType::get(CGM.getLLVMContext(), {
> +  PtrToInt8Ty, // Really a selector, but the runtime creates it us.
> +  PtrToInt8Ty, // Method types
> +  IMPTy// Method pointer
> +});
>auto Methods = MethodList.beginArray();
>for (unsigned int i = 0, e = MethodTypes.size(); i < e; ++i) {
> -llvm::Constant *Method =
> +llvm::Constant *FnPtr =
>TheModule.getFunction(SymbolNameForMethod(ClassName, CategoryName,
>  MethodSels[i],
>  isClassMethodList));
> -assert(Method && "Can't generate metadata for method that doesn't
> exist");
> -llvm::Constant *C = MakeConstantString(MethodSels[i].getAsString());
> -Method = llvm::ConstantExpr::getBitCast(Method,
> -IMPTy);
> -Methods.add(
> -llvm::ConstantStruct::get(ObjCMethodTy, {C, MethodTypes[i],
> Method}));
> +assert(FnPtr && "Can't generate metadata for method that doesn't
> exist");
> +auto Method = Methods.beginStruct(ObjCMethodTy);
> +Method.add(MakeConstantString(MethodSels[i].getAsString()));
> +Method.add(MethodTypes[i]);
> +Method.addBitCast(FnPtr, IMPTy);
> +Method.finishAndAddTo(Methods);
>}
>Methods.finishAndAddTo(MethodList);
>
> @@ -1644,7 +1644,7 @@ llvm::Constant *CGObjCGNU::GenerateClass
>// Fill in the structure
>
>// isa
> -  Elements.add(llvm::ConstantExpr::getBitCast(MetaClass, PtrToInt8Ty));
> +  Elements.addBitCast(MetaClass, PtrToInt8Ty);
>// super_class
>Elements.add(SuperClass);
>// name
> @@ -1673,7 +1673,7 @@ llvm::Constant *CGObjCGNU::GenerateClass
>// sibling_class
>Elements.add(NULLPtr);
>// protocols
> -  Elements.add(llvm::ConstantExpr::getBitCast(Protocols, PtrTy));
> +  Elements.addBitCast(Protocols, PtrTy);
>// gc_object_type
>Elements.add(NULLPtr);
>// abi_version
> @@ -1746,9 +1746,7 @@ CGObjCGNU::GenerateProtocolList(ArrayRef
>  } else {
>protocol = value->getValue();
>  }
> -llvm::Constant *Ptr = llvm::ConstantExpr::getBitCast(protocol,
> -   PtrToInt8Ty);
> -Elements.add(Ptr);
> +Elements.addBitCast(protocol, PtrToInt8Ty);
>}
>Elements.finishAndAddTo(ProtocolList);
>return 

r288269 - Finish adopting ConstantInitBuilder in CGObjCGNU. NFC.

[John McCall via cfe-commits]

Author: rjmccall
Date: Wed Nov 30 14:19:46 2016
New Revision: 288269

URL: http://llvm.org/viewvc/llvm-project?rev=288269=rev
Log:
Finish adopting ConstantInitBuilder in CGObjCGNU.  NFC.

Modified:
cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
cfe/trunk/lib/CodeGen/ConstantBuilder.h

Modified: cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCGNU.cpp?rev=288269=288268=288269=diff
==
--- cfe/trunk/lib/CodeGen/CGObjCGNU.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGObjCGNU.cpp Wed Nov 30 14:19:46 2016
@@ -164,9 +164,8 @@ protected:
   /// Helper function that generates a constant string and returns a pointer to
   /// the start of the string.  The result of this function can be used 
anywhere
   /// where the C code specifies const char*.  
-  llvm::Constant *MakeConstantString(const std::string ,
- const std::string ="") {
-ConstantAddress Array = CGM.GetAddrOfConstantCString(Str, Name.c_str());
+  llvm::Constant *MakeConstantString(StringRef Str, const char *Name = "") {
+ConstantAddress Array = CGM.GetAddrOfConstantCString(Str, Name);
 return llvm::ConstantExpr::getGetElementPtr(Array.getElementType(),
 Array.getPointer(), Zeros);
   }
@@ -1530,23 +1529,24 @@ GenerateMethodList(StringRef ClassName,
   MethodList.addInt(Int32Ty, MethodTypes.size());
 
   // Get the method structure type.
-  llvm::StructType *ObjCMethodTy = llvm::StructType::get(
-PtrToInt8Ty, // Really a selector, but the runtime creates it us.
-PtrToInt8Ty, // Method types
-IMPTy, //Method pointer
-nullptr);
+  llvm::StructType *ObjCMethodTy =
+llvm::StructType::get(CGM.getLLVMContext(), {
+  PtrToInt8Ty, // Really a selector, but the runtime creates it us.
+  PtrToInt8Ty, // Method types
+  IMPTy// Method pointer
+});
   auto Methods = MethodList.beginArray();
   for (unsigned int i = 0, e = MethodTypes.size(); i < e; ++i) {
-llvm::Constant *Method =
+llvm::Constant *FnPtr =
   TheModule.getFunction(SymbolNameForMethod(ClassName, CategoryName,
 MethodSels[i],
 isClassMethodList));
-assert(Method && "Can't generate metadata for method that doesn't exist");
-llvm::Constant *C = MakeConstantString(MethodSels[i].getAsString());
-Method = llvm::ConstantExpr::getBitCast(Method,
-IMPTy);
-Methods.add(
-llvm::ConstantStruct::get(ObjCMethodTy, {C, MethodTypes[i], Method}));
+assert(FnPtr && "Can't generate metadata for method that doesn't exist");
+auto Method = Methods.beginStruct(ObjCMethodTy);
+Method.add(MakeConstantString(MethodSels[i].getAsString()));
+Method.add(MethodTypes[i]);
+Method.addBitCast(FnPtr, IMPTy);
+Method.finishAndAddTo(Methods);
   }
   Methods.finishAndAddTo(MethodList);
 
@@ -1644,7 +1644,7 @@ llvm::Constant *CGObjCGNU::GenerateClass
   // Fill in the structure
 
   // isa 
-  Elements.add(llvm::ConstantExpr::getBitCast(MetaClass, PtrToInt8Ty));
+  Elements.addBitCast(MetaClass, PtrToInt8Ty);
   // super_class
   Elements.add(SuperClass);
   // name
@@ -1673,7 +1673,7 @@ llvm::Constant *CGObjCGNU::GenerateClass
   // sibling_class
   Elements.add(NULLPtr);
   // protocols
-  Elements.add(llvm::ConstantExpr::getBitCast(Protocols, PtrTy));
+  Elements.addBitCast(Protocols, PtrTy);
   // gc_object_type
   Elements.add(NULLPtr);
   // abi_version
@@ -1746,9 +1746,7 @@ CGObjCGNU::GenerateProtocolList(ArrayRef
 } else {
   protocol = value->getValue();
 }
-llvm::Constant *Ptr = llvm::ConstantExpr::getBitCast(protocol,
-   PtrToInt8Ty);
-Elements.add(Ptr);
+Elements.addBitCast(protocol, PtrToInt8Ty);
   }
   Elements.finishAndAddTo(ProtocolList);
   return ProtocolList.finishAndCreateGlobal(".objc_protocol_list",
@@ -1959,11 +1957,11 @@ void CGObjCGNU::GenerateProtocolHolderCa
   Elements.add(MakeConstantString(CategoryName));
   Elements.add(MakeConstantString(ClassName));
   // Instance method list
-  Elements.add(llvm::ConstantExpr::getBitCast(GenerateMethodList(
-  ClassName, CategoryName, MethodSels, MethodTypes, false), PtrTy));
+  Elements.addBitCast(GenerateMethodList(
+  ClassName, CategoryName, MethodSels, MethodTypes, false), PtrTy);
   // Class method list
-  Elements.add(llvm::ConstantExpr::getBitCast(GenerateMethodList(
-  ClassName, CategoryName, MethodSels, MethodTypes, true), PtrTy));
+  Elements.addBitCast(GenerateMethodList(
+  ClassName, CategoryName, MethodSels, MethodTypes, true), PtrTy);
 
   // Protocol list
   ConstantInitBuilder ProtocolListBuilder(CGM);
@@ -1973,15 +1971,13 @@ void CGObjCGNU::GenerateProtocolHolderCa
   auto ProtocolElements =