Update extern(C++) grammar to support string namespaces

[rmanthorpe]

The grammar is missing the string-style extern(C++, "a", "b") declaration. The ArgumentList isn't quite right here as it allows a trailing comma but the current implementation in dmd does not. However, this seems more like a bug in dmd since it's inconsistent with the rest of the grammar.

[dlang-bot]

Thanks for your pull request and interest in making D better, @rmanthorpe! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[Hackerpilot]

I think we may need to introduce a new rule here. The parser looks for a comma-separated list of conditional expressions, while ArgumentList is a comma-separated list of AssignExpression.

[rmanthorpe]

I'm not convinced. As it is it seems to happily parse an AssignExpression. For example extern(C++, (a = "b")) int c(); parses fine, just like the identifier with an assign index works fine: extern(C++, (d[e = f])) int g();

[Hackerpilot]

Yes, extern(C++, (a = "b")) int c(); parses, but extern(C++, a = "b") int c(); does not, which is my point. Putting the assign expression inside the parenthesis makes it match the rule for PrimaryExpression, which causes it to match the rule for ConditionalExpression.

[rmanthorpe]

Ah you're quite right - this is not my forte. In that case, is the identifier list grammar wrong too? extern(C++, d[e = f]) int g(); does not parse.

[rmanthorpe]

@Hackerpilot does that look better now?

[Hackerpilot]

I was going to suggest re-ordering things to make the rule left-recursive, but looking over the grammar I can see that the existing style is that rules are right-recursive. My only suggestion is to remove the redundant NamespaceExpression rule and use ConditionalExpression directly.

[rmanthorpe]

Yes everything is right-recursive so I stuck with the same style. I've removed that redundant expression now.

[rmanthorpe]

Bump @Hackerpilot

[Hackerpilot]

I don't have the ability to merge this.

[PetarKirov]

I can merge, but we need to figure out why the build fails first. I will force push, in order to restart it (assuming it was simply a transient failure).

[rmanthorpe]

I rebased it just to see if that would help but now it's failing circleci too 🤦‍♂ I really have no idea why this would be. Any thoughts?

[Geod24]

Rebased, squashed the commit, and (hopefully) fixed.

I really have no idea why this would be. Any thoughts?

I'm no DDOC expert but I haven't seen any other place in that page where $(D ,) is used. Since it's also used for macro arguments, that might be the source of the error, so I've removed the surrounding $(D ,)

[rmanthorpe]

Rebased, squashed the commit, and (hopefully) fixed.

Thanks I'm totally stumped by this!

[Geod24]

Thanks I'm totally stumped by this!

Well, me too. I found two errors, but it's still red. I'm not 100% sure the $(D ,) would lead to error, but what I found was that GLINK and GLINK2 were inverted. GLINK is for links within the page. Since ConditionalExpression is in expression.dd and not attribute.dd, $(GLINK2 expression, ConditonalExpression) was needed. Additionally, $(GLINK2) was used to link to declaration's NamespaceList, however NamespaceList is in attribute, not declaration, so we should use GLINK instead.

Now, even with that fixed, it is still red, and I have no clue why. I'm on Mac so I can't build ATM, going to give it another try later.

[Geod24]

Yeah, I have absolutely no clue what is wrong. There is so much pre-processing going on that linking the error message with the input is near impossible. I tried a few things, including removing all links, and reducing this until it passes. It did pass when the $(GNAME NamespaceList): and following was removed, but I really can't see anything wrong with it.

@CyberShadow : Would it be possible to have artifacts of failing build be published ? I have no clue what was fed to the LaTeX compiler.

[CyberShadow]

Thanks for looking into this.

@CyberShadow : Would it be possible to have artifacts of failing build be published ? I have no clue what was fed to the LaTeX compiler.

It does do that if the test phase fails, but this is a build problem, and I think the latex input is a temporary file (not part of the output) so it wouldn't be accessible anyway.

The software running the build is based on Digger, so you should be able to reproduce build problems locally by running digger build --with=website master+dlang.org#2716 (you may need to install some packages first).

[Geod24]

Rebased on master, let's see if it is finally green

[Geod24]

Finally! Thanks a lot @rmanthorpe and @MoonlightSentinel . So glad we got rid of that PDF export...