Closed cdriveraus closed 4 years ago
Thanks for letting us know! Yes so there are two things here
.*
on two reals. I think this is a bug and we should support .*
for real * real
But then the generated cpp code fails to compile because of the gcc error from a stray escape character
cstsem.hpp:25001: error: stray ‘\’ in program
I put a versions of your code here that is cleaned up. One has the errors in comments and their fixes and the other uses the new compilers auto-format to make it look pretty. Though not helpful atm
I'll file a separate issue reporting the weird escape \
that needs to be fixed
After looking into the issue this would def effect other folks so thanks for letting us know!! Made #621 that covers the problem
Thanks @cdriveraus !
2. In the new compiler right now you can't have variables that shadow stan function names. There's a PR right now #569 to fix this
Which name is the issue? AFAIK stanc2 also didn't allow shadowing across the board. Example:
real foo(real max /* this one is fine */) {
real min; // this isnt
...
}
offset
and multiplier
though are reserved words in stanc3. That is not going away or at least hasn't been discussed.
Ah okay then ignore that part of my comment, forgot those are reserved words now
Hah. Good to see my suggested terminology coming back to break my own program. Is it really necessary for offset and multiplier to be reserved? I thought they were only relevant in the very specific < > context. I assume there's a reason but ask in case :)
I don't understand the stray \ thing -- is this an issue with my code or something to do with the new parser?
I don't understand the stray \ thing -- is this an issue with my code or something to do with the new parser?
This is a parser thing nothing on your end
Is it really necessary for offset and multiplier to be reserved? I thought they were only relevant in the very specific < > context. I assume there's a reason but ask in case :)
I'll leave that to other folks I can't comment on that
The original issue for that topic was https://github.com/stan-dev/stan/issues/2712 and then some additional stuff in https://github.com/stan-dev/stanc3/issues/453
The gist of it I think is this quote: "Currently, various keywords can be used as variable names in Stan. This seems like a bad idea from a PL perspective."
It does bring up the question of whether this breaks backward compatibility.
Yeah... from where I sit it seems like pretty limited discussion / justification for breaking changes, but then I'm no language designer. For context, my own software uses these terms in functions, and these functions also get parsed into stan code. I don't think a fix will be hard but it will be a little inelegant so long as I stay backwards compatible. I also don't think useful progress should be held up because of little things like this, just not obvious to me if it's useful progress :)
but then I'm no language designer
Neither am I, just posting what I know/remember. I
The fix would not be that hard, but I am guessing the language designers had their own valid reasons. Let's wait for some of them chime in.
Is it really necessary for offset and multiplier to be reserved?
I don't think they should be reserved. I also don't think lower
and upper
need to be reserved.
I am going to close this issue and open issues for the remaining problems that have not been resolved:
I think its more likely things get fixed this way than someone going down looking at what is still to fix here.
Following up on this issue. This model now compiles with the latest version of the stanc.js
According to rstan, the following model will not work in future: