test suite 'owner'

Hi,

I have found it necessary to make some changes to the test-suite for the XCore platform.

These changes include:
altering #includes, as supported by XCore;
using stdout or stderr to make the output diffs consistent (fixing expected output too);
(This work is still under review as best way to do it)
‘fixing’ symbol and type problems e.g name clashes & scope;
altering/adding code snippets and macros.

I have used #ifdef to limit and keep any changes specific to the XCore.
Some of these could/should be made common to all targets e.g. log2() → logTwo().

I have also altered the Makefile to filter out tests not supported by the XCore.

I would like to discuss the changes I have found necessary to make and what is the next step.
Should any/all of them be pushed upstream?

Robert

PatchTest-suite (37.1 KB)

Some of these are pretty weird, e.g. int32_t main. Probably the best thing is to submit each patch individually with an explanation of what the purpose is and we can talk about them then.

-eric

Hi Eric,

Could you explain the intent and policy regarding the test-suite body of code.
Should the test be left as much as possible as-is (even if technically incorrect)?
Should changes only affect the XCore target (#ifdef) or should all targets get the changes?

Taking “int32_t main” as an example.
The correct return type & argc for main is ‘int’.
In the XCore tool chain, ‘int32_t’ equates to long (IIRC) and hence is not acceptable in the type signature for main.
Should this change be only for the XCore target or all targets?

When I know the policy for the test-suite, I’ll alter as necessary & regroup the changes into patches containing the same type of change and submit for approval.

One more question:
On patch I need to address is how to make deterministic the order of stdout & stderr.
Ideally, applications would use either stdout or stderr but not both.
Would a patch to change to only stdout be acceptable (plus any changes to expected output)?

Thank you

Robert

The idea is that it’s going to be a correct and portable set of code that works both as a correctness and performance test suite.

-eric

… and so (I infer from that) it should not be patched let alone need any changes.

Assuming my inference is correct, any patching should only affect the XCore target and only if there is a good reason why the XCore requires the change.

So, is #ifdef around all/most changes the correct way to submit a patch?

Robert

... and so (I infer from that) it should not be patched let alone need any
changes.

There are the occasional system level changes to it, but they're rare.

Assuming my inference is correct, any patching should only affect the XCore
target and only if there is a good reason why the XCore requires the change.

So, is #ifdef around all/most changes the correct way to submit a patch?

It's painful, but if there's no other way...

-eric

Hi Eric,

Could you explain the intent and policy regarding the test-suite body of code.
Should the test be left as much as possible as-is (even if technically incorrect)?
Should changes only affect the XCore target (#ifdef) or should all targets get the changes?

Taking “int32_t main” as an example.
The correct return type & argc for main is ‘int’.
In the XCore tool chain, ‘int32_t’ equates to long (IIRC) and hence is not acceptable in the type signature for main.
Should this change be only for the XCore target or all targets?

All targets. This code is incorrect/non-portable. The return type of main is ‘int’ per the language spec.

Eric,
Thank you.
I'll start to present them for review.
Robert

thank you.
I’ll submit the patch without #ifdef in this case.
Robert

I have unbundled and submitted the first 4 patches.
Please see:

http://llvm-reviews.chandlerc.com/D2615
[PATCH] test-stuite: Fix signature of main() to use ‘int’ rather than ‘int32_t’

http://llvm-reviews.chandlerc.com/D2617
[PATCH] test-suite: Fix name clash ‘infinity’

http://llvm-reviews.chandlerc.com/D2618
[PATCH] test-suite: Fix name clash ‘log2’

http://llvm-reviews.chandlerc.com/D2619
[PATCH] test-suite: XCore target: fix handling of <string.h> and <memory.h>

Thank you.

Robert

If you haven’t already, you probably want to add llvm-commits as a reviewer or cc on these code reviews so they can be reviewed in public.

thank you!
Added to CC.
Not sure who I should request as a reviewer.

Robert