Closed m8pple closed 3 years ago
It happens with composite initialisers too:
# > /mnt/e/dt10_all/POETS/Orchestrator/Output/Composer/apsp_100_10__apsp_100_10/Generated/src/vars_0_0.cpp:1612:657: error: braces around scalar initializer for type 'uint32_t' {aka 'long unsigned int'}
# > devtyp_control_fanout_props_t Thread_0_DeviceProperties[99] = {{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}},{{2}}};
# > ^
with type:
typedef struct control_fanout_properties_t
{
// Line 48
uint32_t degree;
} devtyp_control_fanout_props_t;
# > /mnt/e/dt10_all/POETS/Orchestrator/Output/Composer/apsp_100_10__apsp_100_10/Generated/src/vars_0_0.cpp:1614:2236: error: braces around scalar initializer for type 'uint32_t' {aka 'long unsigned int'}
# > devtyp_control_fanout_state_t Thread_0_DeviceState[99] = {{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}},{{0,0,0,0,0,0,0,0,0}}};
# > ^
with type:
typedef struct control_fanout_state_t
{
// Line 58
uint32_t responseSentCount;
uint32_t responseReceivedCount;
uint32_t responseSeen;
uint32_t responseSumDistance;
uint32_t responseMaxDistance;
uint32_t round;
uint32_t requestValid;
uint32_t requestRound;
uint8_t requestIsNewRound;
} devtyp_control_fanout_state_t;
I think this is due to a difference of reading how typedDataSpec values are defined. To be fair the grammar is ambiguous, though the examples all seem to go one way.
The approach used in all the examples in PIP20 is that values given for properties and state should literally just be the initialiser one would paste into C to initialise the type, as that seemed most in line with the token pasting approach that was desired. So because the typed data spec always defines a struct, the PIP20 XML examples from IC all have the brace initialiser.
In the orchestrator the (entirely reasonable) approach is to accept that the braces are always there, and so we should omit them - saving chars in the largest part of the file.
This was also the approach taken in v3 with the initialisers using embedded JSON, but the addition and removal of brackets was seen as clumsy, so it was removed in all the PIP20 IC examples as it didn't seem as close to the "just make everything C tokens". On the other hand the typedDataSpec explicitly doesn't have the surrounding braces.
So in Soton XML it is:
<GraphInstance id="plate_3x3" graphTypeId="plate_heat" P="3,3,9">
<DeviceInstances>
<DevI id="c_0_0" type="fixedNode" P="1000.000000,0,0"/>
<DevI id="c_0_1" type="cell" P="0,1,0,0.031250"/>
but in IC XML it would be:
<GraphInstance id="plate_3x3" graphTypeId="plate_heat" P="{3,3,9}">
<DeviceInstances>
<DevI id="c_0_0" type="fixedNode" P="{1000.000000,0,0}"/>
<DevI id="c_0_1" type="cell" P="{0,1,0,0.031250}"/>
I'm not sure how to resolve this, as there is a lot of XML floating around that uses the extra initialisers from IC, and also a lot from Soton that doesn't.
In graph_schema parsers it would be possible to compare the structure of the type against the structure of the value, because it parses both of them, it could then select the single approach that fits - fair amount of work though.
In the orchestrator I don't think it is that easy, because it never examines the structures and values, so it will be difficult to work out which format it is in.
The hacky method in the commit df8099dc58b45fe6719fbc17a2e9eee9a43b0bf5 applies the following rule:
{}
{0,1,2}
{{3,2},1,2}
0
{0},1,2
{{4,3,n}}
That last case {{4,3,n}}
is the only real problem: if someone declares a tuple as the child of a Message,
Properties, or State node then the heuristic will fail.
More concrete ways forward:
(Note that I think the "no-surrounding brackets" approach makes more sense, and it is the one we were using until the v4 standardisation changed it).
This pretty easy for IC parsers and generators. I looked through the code, and auto-switching is around 10 lines of code, purely in the parser front-end.
For the Soton parser I think this is much more difficult, as it treats structure definitions and values as strings (by design). Practically speaking I don't think this is feasible, as it would push orchestrator adoption back even further.
It would be possible to create a streaming "surrounding|no-surrounding" -> "no-surrounding"
filter as a standalone process. This could then be used as a filter through popen
.
After... time, it seems like there is an fopen that happens here:
or maybe not... I really had difficulty following the call changes. But if so, it could spawn a filter through popen.
It would have some perf overhead, but would mean that the orchestrator changes would be minimal.
Not too difficult for graph_schema, and minimal changes for orchestrator. I'm feeling this might be inevitable anyway.
But the heuristic patch works around it in the short term, so it might distract from just getting things working and bringing DPD up.
In the orchestrator the (entirely reasonable) approach is to accept that the braces are always there, and so we should omit them - saving chars in the largest part of the file.
I think that your comment nails the rational quite well, though their dropping was an edict from on high of "why do we need them?". That said, it is only two characters per initialiser.
I think this is due to a difference of reading how typedDataSpec values are defined.
I would be inclined to agree, the line scalar literals **and** braced lists
(my emphasis) can easily be read as not needing the braces.
I'm not sure how to resolve this, as there is a lot of XML floating around that uses the extra initialisers from IC, and also a lot from Soton that doesn't.
There is not that much from us without them - a handful of small examples with very few pins plus heated plate and reactive (both of which are formed from generators that would only need a couple of lines change to add in these braces).
Actual implementation on the Orchestrator would be a change in Composer to not add the braces.
I have an alternative suggestion on the way forward:
This obviously means updating our examples and generators. To make the transition easier on this end, I could modify Composer to look at the first and last characters of the initialiser and add braces if they are not already braces. Obviously this does not work for a case where there is a tuple at the start and end of the state/etc. (e.g. {1,2},{3,4}
) but it would catch most cases.
I will wait for @mvousden and @DrongoTheDog to chime in before we finalise anything.
Option 4 definitely makes things easier (though I do actually feel that the approach taken by the orchestrator is actually better (that's why it was done that way in v3)).
make the transition easier on this end, I could modify Composer to look at the first and last characters of the initialiser and add braces if they are not already braces. Obviously this does not work for a case where there is a tuple at the start and end of the state/etc. (e.g. {1,2},{3,4}) but it would catch most cases.
The approach I took with df8099dc58b45fe6719fbc17a2e9eee9a43b0bf5 should (?) already handle this,
as it parses/matches the opening and closing brackets. So it can detect almost all cases except for things
like {{4,3,n}}
, where someone has a tuple as the single sub-child of a Properties or State.
With that patch, I have the orchestrator parsing all v4 XML I give it. There are compilation errors, but I haven't dug into those yet. Quite a lot of IC v4 XML now parses and compiles correctly.
I will wait for @mvousden and @DrongoTheDog to chime in before we finalise anything.
Does @DrongoTheDog actually do github?
Coo. I've never thought of myself as on high.
Mixing ones metaphors, I think this is a rabbit hole with a swamp in it.
I subscribe to the idea of separation of concerns. The Orchestrator XML is a wrapper that contains all sorts of stuff: topology graphs, type definition trees, metadata - whenever that crystallises - and so on. There's an XML syntax, and an XML semantic checker. There's also a strange and mysterious construct called CDATA, which departs from the syntax of XML and contains semantics that are Of No Concern to the XML world. The Orchestrator takes the contents of the CDATA blocks - whatever they may be - and bolts them together in a framework (the softSwitch) under the control of directives that are the XML. What is actually in those blocks is not the Orchestrators problem; it's the cross-compilers problem.
The Orchestrator should deal with the XML, the cross-compiler with the source code.
Once you start trying to introspect the code (and right away we're assuming it's C - hmmm) where do we stop? Pairing up the braces? Checking def/refs? Illegal operators? Malformed expressions?
The Orchestrator should deal with the XML, the cross-compiler with the source code.
There's enough hooks in the XML/CDATA to let the user take back-bearings from the softswitch source (i.e. cross-compiler output) to the XML source. That's everything you need, it's all you need, and nothing you don't.
ADB
Andrew Brown Professor of Electronics University of Southampton Hampshire SO17 1BJ UK
T: 02380 593374 M: 07464 981171
@DrongoTheDog 's last comment on this is endlessly interesting, but actually has little bearing on this issue, and seems to have stalled progress on resolving it.
So I previously wrote and proposed a patch - it's not great, but it works. I would
prefer not to use it, but at the moment I have to keep these side patches that I
keep overlaying on development
, as otherwise it is not possible to parse the
v4 xml test-suite.
While omitting the brackets is probably the right thing to do, I think actually implementing it is going to take too long.
So in the interests of just picking something and moving on, I strongly suggest that we just modify the Orchestrator to expect surrounding braces. It's an easy fix, and is implemented in #264
resolved in #264
Compilation fails due to extra braces:
Orchestrator version is dt10_development_branch at 6e2198fd599a983f3f5efeaab67906e2d679469a - code gen is the same as 1.0.0-alpha head.
Source file is https://github.com/POETSII/Orchestrator/blob/6e2198fd599a983f3f5efeaab67906e2d679469a/Tests/ReferenceXML/v4/PEP20/apps/amg_poisson_8_8_v4.xml
The source line that is going wrong is:
The declaration of global_props_t is:
This extra braces problem occurs with initialisers in many cases, and is one of the most common compilation problems I see so far.
I couldn't see any other issues mentioning it, though only searched for the error message components.