[PATCH] D113566: [fir] Add fir.box_isarray, fir.box_isptr and fir.box_isalloc conversion.

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 2 inline comments as done.
clementval added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:34-35
+/// `fir.box` attribute values as defined in flang/ISO_Fortran_binding.h.
+static constexpr unsigned kAttrPointer = CFI_attribute_pointer;
+static constexpr unsigned kAttrAllocatable = CFI_attribute_allocatable;
+
----------------
awarzynski wrote:
> clementval wrote:
> > clementval wrote:
> > > awarzynski wrote:
> > > > Why not use the macro directly?
> > > `fir.box` is an abstraction of a descriptor also we can get a compile time error in case the macro is not a uint. 
> > also it makes it homogenous with the `fir.box` field indexing added previously so the code is maybe easier to understand. 
> > we can get a compile time error in case the macro is not a uint
> 
> Any idea why wouldn't the original value be define as `uint`? That would make it easier for you here.
> 
> > also it makes it homogenous with the fir.box field indexing added previously so the code is maybe easier to understand.
> 
> Agreed, this does help a lot!
> 
> I think that you are doing a great job and a massive favour to all of us here. I'm just thinking that perhaps stuff  from "flang/ISO_Fortran_binding.h" should be wrapped in an enum class or something similar? Basically, sth to facilitate code re-use a bit better. WDYT? 
> > we can get a compile time error in case the macro is not a uint
> 
> Any idea why wouldn't the original value be define as `uint`? That would make it easier for you here.

Who knows what people can do. 

> 
> > also it makes it homogenous with the fir.box field indexing added previously so the code is maybe easier to understand.
> 
> Agreed, this does help a lot!
> 
> I think that you are doing a great job and a massive favour to all of us here. I'm just thinking that perhaps stuff  from "flang/ISO_Fortran_binding.h" should be wrapped in an enum class or something similar? Basically, sth to facilitate code re-use a bit better. WDYT? 

This could be an improvement but it's not something that's in the scope of this patch and will likely require some effort in fir-dev first. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113566