[PATCH] D112381: [flang] Checks for pointers to intrinsic functions

[Emil Kieri via Phabricator via llvm-commits]

ekieri marked 3 inline comments as done.
ekieri added a comment.

Thanks @klausler for your comments!

I do not have commit access. If this looks ok to you, could you land the patch for me? Please use “Emil Kieri <j.emil.kieri at gmail.com>” to commit it.

Cheers, Emil



================
Comment at: flang/lib/Evaluate/characteristics.cpp:421
               // declaration checking in Semantics.
-              return context.intrinsics().IsSpecificIntrinsicFunction(
-                  symbol.name().ToString());
+              if (const auto intrinsic{
+                      context.intrinsics().IsSpecificIntrinsicFunction(
----------------
klausler wrote:
> Possibly easier to follow:
> 
>   auto intrinsic{...};
>   if (intrinsic && intrinsic->isRestrictedSpecific) {
>     intrinsic.reset(); // exclude intrinsics from table 16.3
>   }
>   return intrinsic.
> 
Yes, thanks!


================
Comment at: flang/lib/Semantics/check-declarations.cpp:636
                 ultimate.name(), symbol.name());
           }
         } else if (!ultimate.attrs().test(Attr::EXTERNAL) &&
----------------
klausler wrote:
> You could emit distinct messages for the two possible error cases (!intrinsic for one, intrinsic->isRestrictedSpecific for the other).
> 
> If you choose to emit just the one message, the predicate could be made easier to follow as:
> 
>   !intrinsic || intrinsic->isRestrictedSpecific
> 
> to avoid making the reader of the code have to figure out multiple negations.
I would prefer to stay with just the one message. I think that better matches the actual condition (must be listed in 16.2), handling the cases separately would imho follow the implementation details rather than the underlying logic.

Thanks for the tip with the predicate! Looks cleaner, double negations should clearly be avoided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112381