[PATCH] D137725: [OpenMP][OMPIRBuilder] Mirgrate getName from clang to OMPIRBuilder

[Jan Sjödin via Phabricator via llvm-commits]

jsjodin added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:84
+  /// Separator used between all of the rest consecutive parts of s name
+  StringRef Separator;
+
----------------
jdoerfert wrote:
> This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?
> This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?




================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:84
+  /// Separator used between all of the rest consecutive parts of s name
+  StringRef Separator;
+
----------------
jsjodin wrote:
> jdoerfert wrote:
> > This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?
> > This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?
> 
> 
> This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?

Yes, I updated the patch just to see that everything builds okay, but we should be able to infer the separators.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:117
+  /// \param Parts different parts of the final name that needs separation
+  std::string getName(ArrayRef<StringRef> Parts) const;
+
----------------
jdoerfert wrote:
> Rename to something meaningful.
> Rename to something meaningful.

Forgot to address this, will fix together with inferring separators.



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

https://reviews.llvm.org/D137725