https://issues.dlang.org/show_bug.cgi?id=23520

Iain Buclaw <ibuclaw@gdcproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
                 CC|                            |ibuclaw@gdcproject.org

--

https://issues.dlang.org/show_bug.cgi?id=23520

RazvanN <razvan.nitu1305@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |razvan.nitu1305@gmail.com

--- Comment #1 from RazvanN <razvan.nitu1305@gmail.com> ---
It seems that the compiler has PragmaDeclaration and PragmaStatement to differentiate between pragmas that are declared at module/aggregate level and pragmas that are declared in function bodies. If you have any kind of pragma that is inside a function body it will then be seen as a pragma statement, irrespective of the fact that it is attached to a single declaration or to a block of statements.

I see 2 ways of fixing this:

1. We can implement the expected behavior by attaching the pragma to the declaration, but given the existing machinery in the compiler this will require a lot of rework to transform the existing pragma statement into a pragma declaration and hook in to the existing implementation.

2. Simply deprecate using pragma(inline) statements attached to declarations at statement level. The online pragma that should be allowed at statement level is `pragma(inline, expression);` In the case provided in the bug report you can simply move the pragma(inline) inside the noinline function and it will work. This has the advantage of keeping the implementation simple.

I would lean towards the second resolution as it keeps things simple. What do you think Iain?

--

https://issues.dlang.org/show_bug.cgi?id=23520

--- Comment #2 from Iain Buclaw <ibuclaw@gdcproject.org> ---
(In reply to RazvanN from comment #1)
> 1. We can implement the expected behavior by attaching the pragma to the declaration, but given the existing machinery in the compiler this will require a lot of rework to transform the existing pragma statement into a pragma declaration and hook in to the existing implementation.
Why do you think so?

The parser looks like this:
---
if (token.value == TOK.semicolon)
{
    nextToken();
    _body = null;
}
else
    _body = parseStatement(ParseStatementFlags.semi);
s = new AST.PragmaStatement(loc, ident, args, _body);
break;
---

A standalone `pragma(inline);` statement will have no `_body`.  This can be checked at semantic with no invasive changes.

--

https://issues.dlang.org/show_bug.cgi?id=23520

--- Comment #3 from RazvanN <razvan.nitu1305@gmail.com> ---
(In reply to Iain Buclaw from comment #2)
> (In reply to RazvanN from comment #1)
> > 1. We can implement the expected behavior by attaching the pragma to the declaration, but given the existing machinery in the compiler this will require a lot of rework to transform the existing pragma statement into a pragma declaration and hook in to the existing implementation.
> Why do you think so?
> 
> The parser looks like this:
> ---
> if (token.value == TOK.semicolon)
> {
>     nextToken();
>     _body = null;
> }
> else
>     _body = parseStatement(ParseStatementFlags.semi);
> s = new AST.PragmaStatement(loc, ident, args, _body);
> break;
> ---
> 
> A standalone `pragma(inline);` statement will have no `_body`.  This can be checked at semantic with no invasive changes.

That is true, but in your original bug report you want to apply the pragma to the declarations inside the body, right? To be able to do that you need to create a PragmaDeclaration and attach it to the the scope of the pragma statement body (struct Scope has a field inlining of type PragmaDeclaration).

--

https://issues.dlang.org/show_bug.cgi?id=23520

--- Comment #4 from RazvanN <razvan.nitu1305@gmail.com> ---

> 
> That is true, but in your original bug report you want to apply the pragma to the declarations inside the body, right? To be able to do that you need to create a PragmaDeclaration and attach it to the the scope of the pragma statement body (struct Scope has a field inlining of type PragmaDeclaration).

Of course we can implement a suboptimal algorithm where we go through the members of the body and simply update the inlining field for functions. But then the members will be reiterated for semantic analysis. I guess in the general case this should not be a problem as I do not expect large pragma(inline) blocks inside function bodies.

--

https://issues.dlang.org/show_bug.cgi?id=23520

--- Comment #5 from Iain Buclaw <ibuclaw@gdcproject.org> ---
(In reply to RazvanN from comment #3)
> That is true, but in your original bug report you want to apply the pragma to the declarations inside the body, right? To be able to do that you need to create a PragmaDeclaration and attach it to the the scope of the pragma statement body (struct Scope has a field inlining of type PragmaDeclaration).
Yes, that can be handled by the statement semantic for PragmaStatement.

I think something like the following should suffice:
---
Scope* sc2 = sc;
scope(exit)
    if (sc2 != sc)
        sc2.pop();
// ...
if (ps.ident == Id.Pinline)
{
    if (auto fd = sc.func)
    {
        if (ps._body)
            sc2 = new PragmaDeclaration(...).newScope(sc);
        else
            fd.inlining = evalPragmaInline(ps.loc, sc, ps.args);
    }
    else
    {
        ps.error("`pragma(inline)` is not inside a function");
        return setError();
    }
}
// ...
if (ps._body)
{
    ps._body = ps._body.statementSemantic(sc2);
}

--

https://issues.dlang.org/show_bug.cgi?id=23520

--- Comment #6 from Iain Buclaw <ibuclaw@gdcproject.org> ---
Saying that... the way that pragmas are handled in the front-end is all over the shop, with no coherent way to manage them (is a pragma standalone? must have a body? statement only? declaration only? etc...), or allow vendors to extend them (without forking away from upstream dmd) despite this being the intent of pragmas in the spec.

See also issue 19109 and issue 19110.

--

https://issues.dlang.org/show_bug.cgi?id=23520

--- Comment #7 from Dlang Bot <dlang-bot@dlang.rocks> ---
@ibuclaw created dlang/phobos pull request #8642 "std.math.hardware: Fix broken IEEE FP flags tests" mentioning this issue:

- std.math.hardware: Fix broken IEEE FP flags tests

  1. The `pragma(inline, false)` doesn't do what the author thinks it does
     (Issue 23520).
  2. Change that introduced `blockopt` seems to have a slight
     misunderstanding about what constant propagation is (`a /= 0.0L` can
     always be constant propagated regardless of what surrounds it).

https://github.com/dlang/phobos/pull/8642

--

https://issues.dlang.org/show_bug.cgi?id=23520

--- Comment #8 from Dlang Bot <dlang-bot@dlang.rocks> ---
dlang/phobos pull request #8642 "std.math.hardware: Fix broken IEEE FP flags tests" was merged into master:

- 36b2f9c6aa3d705c9b2c019ebf4480b5980502ad by Iain Buclaw:
  std.math.hardware: Fix broken IEEE FP flags tests

  1. The `pragma(inline, false)` doesn't do what the author thinks it does
     (Issue 23520).
  2. Change that introduced `blockopt` seems to have a slight
     misunderstanding about what constant propagation is (`a /= 0.0L` can
     always be constant propagated regardless of what surrounds it).
  3. Undocument tests that depend on the need for forcing operations to
     disarm the optimizer, and replace with simple tests behind StdDdoc.

https://github.com/dlang/phobos/pull/8642

--

https://issues.dlang.org/show_bug.cgi?id=23520

Iain Buclaw <ibuclaw@gdcproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P3

--