[PATCH] D118276: Adding a DIBuilder interface for Fortran's assumed length string

[Brock Wyma via Phabricator via llvm-commits]

bwyma added inline comments.


================
Comment at: llvm/include/llvm/IR/DIBuilder.h:237
     /// \param StrLocationExp   Optional memory location of the string.
-    DIStringType *createStringTypeExp(StringRef Name,
+    DIStringType *createStringType(StringRef Name,
                                       DIExpression *StringLengthExp,
----------------
cchen15 wrote:
> bwyma wrote:
> > cchen15 wrote:
> > > One last suggestion: We can combine the two new functions into one by typing the length argument PointerUnion<DIVariable*, DIExpression*>.  Please see DIBuilder::createArrayType for a precedent.
> > In the createArrayType() instance, the expression and variable arguments are interchangeable metadata to the same composite type parameter. In DIStringType, at least currently, the variable string length and string length expression are independent parameters. To reflect this in the DIBuilder interface I would prefer to have independent createStringType() calls.
> DIStringType should have only one field to represent the string length. I think this boils down to if the DIBuilder interface needs to reflect the current state of the underlying implementation. My take is that we can go ahead with what the user should see with this patch, and then change the implementation at a later time if necessary to avoid an additional API change down the road.
> 
Even if this change was made, there will still be a second interface routine for creating constant string length types. I don't see value in upcasting the arguments and performing isa comparisons on them just to downcast them into separate arguments again. The string type does not yet have the complexity of the array type interface. The proposed implementation here is cleaner and functionally equivalent.


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

https://reviews.llvm.org/D118276