[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Jonathon Penix via Phabricator via cfe-commits]

jpenix-quic added a comment.



> BTW, can you continue working on the lowering of the convert argument of open statement?

I will take a look at this!



================
Comment at: flang/lib/Lower/Bridge.cpp:2757
 
+    if (funit.isMainProgram()) {
+      auto conversion = bridge.getConversion();
----------------
peixin wrote:
> I think this should be moved into `void lowerFunc(Fortran::lower::pft::FunctionLikeUnit &funit)`.
I ended moving this again while changing approaches--the equivalent line is above at lines 243-252. Does the new location seem reasonable? I had to add the bool to indirectly track if a main program was specified, but the new location seemed the most fitting to me. 


================
Comment at: flang/runtime/main.cpp:55
+
+  if (auto convert{Fortran::runtime::GetConvertFromInt(i)}) {
+    Fortran::runtime::executionEnvironment.conversion = *convert;
----------------
peixin wrote:
> Nit: Is it better to to check the range of i before getting it from `GetConvertFromInt`?
> ```
> if (i < static_cast<int>(Convert::Unknown) || i > static_cast<int>(Convert::Swap)) {
>   crash
> } else {
>   Fortran::runtime::executionEnvironment.conversion = static_cast<Convert>(i);
> }
> ```
I ended up not needing `GetConvertFromInt` when reworking the approach, so this has been removed!


================
Comment at: flang/runtime/main.cpp:58
+  } else {
+    Fortran::runtime::Terminator{__FILE__, __LINE__}.Crash(
+        "Invalid convert option passed to ConvertOption");
----------------
peixin wrote:
> Should `__FILE__, __LINE__` be passed as argument in lowering to point to the file name and line of the source file? Or is this (__FILE__, __LINE__) pointing the the file name and line of the source file?
I removed this instance of `__FILE__, __LINE__`, but I added another similar one in `environment.cpp` line 33! 

The error points to `environment.cpp` line 33 in the new revision. I'm not sure if this is the best way of handling it, but given that there isn't a Fortran source line associated with either the environment list or the old runtime call (and as I think both are closer to implementation details than user-facing features), pointing to the runtime file seemed like the least confusing option for the error location.


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

https://reviews.llvm.org/D130513