Disambiguate utest::v1 types and remove using statement #4071

mazimkhan · 2017-03-29T23:59:56Z

Notes:

Pull requests will not be accepted until the submitter has agreed to the contributer agreement.
This is just a template, so feel free to use/remove the unnecessary things

Description

Removed using namespace utest::v1 and types from the namespace Ex: utest::v1::status_t. To avoid ambiguity with same name types.

Status

READY/IN DEVELOPMENT/HOLD

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

YES | NO

Related PRs

List related PRs against other branches:

branch	PR
other_pr_production	link
other_pr_master	link

Todos

Tests
Documentation

Deploy notes

Notes regarding the deployment of this PR. These should note any
required changes in the build environment, tools, compilers, etc.

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

@RonEld @sbutcher-arm @andresag01 Please review.

RonEld · 2017-03-30T08:57:51Z

TESTS/mbedtls/ecp/main.cpp

@@ -20,8 +20,6 @@
 #include "utest.h"
 #include "rtos.h"

-using namespace utest::v1;
-


I would have moved this statement to line 41, instead of removing it, after all the header file includes. In this way, this specific .c file would still be using the namespace, but it won't affect all the header files.
The issue was that this statement was before the header files

It is not a good idea to use the using statement as in future the test case might want to use the other status_t type. And it would be ambiguous.

I understand your point, but in our specific case, status_t was platform specific, so there won't be a use for an other status_t. My main concern is that if new tests will be introduced, one should also add the namsepcae prefix.
My comment wasn't a request for change, it was more of an obeservation

If the other status_t was also in a namespace then using using on one might be ok. using namespace utest::v1 statement may not be a problem now but can be in future. Hence a maintenance thing.
Secondly, I didn't have partner code to verify the impact of moving the using. Hence I went for a more robust approach.

I'm fine with your approach, I only add a statement that future functions\types should also include the utest::v1 prefix.

I also discovered that status_t is also defined in couple of targets and through some include path already available in the test case.

Yes sure I agree.

RonEld · 2017-03-30T08:58:47Z

TESTS/mbedtls/selftest/main.cpp

@@ -20,7 +20,6 @@
 #include "utest.h"
 #include "rtos.h"

-using namespace utest::v1;


same as previous comment.
I would have moved this statement to line 51, instead of removing it, after all the header file includes. In this way, this specific .c file would still be using the namespace, but it won't affect all the header files.
The issue was that this statement was before the header files

same as above.

same as above

RonEld

I approve this PR, however I haven't checked that all types that weren't added the utest::v1 namespace prefix don't need this prefix. It is important to add this prefix for future new tests\functions

mazimkhan · 2017-03-30T09:41:54Z

@RonEld we don't have ensure by review if we missed scope resolution on any type after removing the using statement. Because compiler checks it.

0xc0170 · 2017-03-30T16:53:50Z

/morph test

mbed-bot · 2017-03-30T23:58:54Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1803

Test failed!

bridadan · 2017-03-31T14:18:30Z

retest uvisor

bridadan · 2017-03-31T14:18:58Z

morph failure was due to a board's interface having issues, restarting now.

/morph test

mbed-bot · 2017-03-31T15:53:47Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1807

All builds and test passed!

0xc0170 · 2017-04-03T10:09:04Z

retest uvisor

sg- · 2017-04-06T15:39:30Z

@mazimkhan Please update to move the using statement after all the header inclusions as suggested by @RonEld

mazimkhan · 2017-04-06T16:08:15Z

@sg- I have removed the using statement altogether. Please see the discussion above.

sg- · 2017-04-07T15:57:34Z

Yes but that solution doesn't follow how using is used in other mbed OS tests. All that should be required is moving the using statement below all headers

0xc0170 · 2017-04-10T11:06:06Z

Yes but that solution doesn't follow how using is used in other mbed OS tests. All that should be required is moving the using statement below all headers

@mazimkhan What do you think?

pan- · 2017-04-13T16:42:04Z

@mazimkhan using directives and definitions should be put after the inclusion of all headers otherwise it can pollutes declaration exposed in the headers included after the using statement.

Occurrence to status_t are already qualified so there won't be any issue with potential definitions of status_t present in the global namespace.

If new changes produce clash between symbols present in the global namespace and symbols in utest::v1 namespace when these test files are compiled then it will be possible to do the right thing: ask the producer of the changes to prefix or put into the appropriate namespace its conflicting types.

mazimkhan · 2017-04-18T09:32:49Z

Ok. Moved using statement instead of removing.

sg- · 2017-04-19T04:19:32Z

/morph test

mbed-bot · 2017-04-19T08:45:38Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 33

All builds and test passed!

adbridge · 2017-05-02T14:53:21Z

@theotherjimmy why did you added this to release-version: 5.4.5 when the code is not on the master branch?? Removing the label...

theotherjimmy · 2017-05-02T14:57:19Z

I did not see that. my bad.

0xc0170 added the needs: review label Mar 30, 2017

0xc0170 requested review from simonbutcher and RonEld March 30, 2017 08:48

RonEld reviewed Mar 30, 2017

View reviewed changes

RonEld approved these changes Mar 30, 2017

View reviewed changes

RonEld mentioned this pull request Mar 30, 2017

Fix to the mbedtls test to resolve namespace conflict when running on Kinetis devices #4077

Closed

0xc0170 approved these changes Mar 30, 2017

View reviewed changes

0xc0170 removed the needs: review label Mar 30, 2017

0xc0170 added the needs: CI label Mar 30, 2017

0xc0170 added ready for merge and removed needs: CI labels Apr 3, 2017

sg- added needs: work and removed ready for merge labels Apr 6, 2017

Move using namespace utest::v1 after includes

ee1019d

mazimkhan force-pushed the mbedtls-partner-workshop-17Q2 branch from ecfe60e to ee1019d Compare April 18, 2017 09:30

sg- added the needs: CI label Apr 19, 2017

sg- removed the needs: work label Apr 19, 2017

bridadan added needs: review and removed needs: CI labels Apr 19, 2017

theotherjimmy added ready for merge and removed needs: review labels Apr 24, 2017

theotherjimmy merged commit a7d7f92 into ARMmbed:mbed-os-workshop-17q2 Apr 24, 2017

theotherjimmy added release-version: 5.4.5 and removed ready for merge labels Apr 24, 2017

adbridge removed the release-version: 5.4.5 label May 2, 2017

c1728p9 mentioned this pull request Jun 7, 2017

Add mbedtls tests #4498

Closed

Disambiguate utest::v1 types and remove using statement #4071

Disambiguate utest::v1 types and remove using statement #4071

Uh oh!

Conversation

mazimkhan commented Mar 29, 2017

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazimkhan Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

mazimkhan commented Mar 30, 2017

Uh oh!

0xc0170 commented Mar 30, 2017

Uh oh!

mbed-bot commented Mar 30, 2017

Output

Uh oh!

bridadan commented Mar 31, 2017

Uh oh!

bridadan commented Mar 31, 2017

Uh oh!

mbed-bot commented Mar 31, 2017

Output

Uh oh!

0xc0170 commented Apr 3, 2017

Uh oh!

sg- commented Apr 6, 2017

Uh oh!

mazimkhan commented Apr 6, 2017

Uh oh!

sg- commented Apr 7, 2017

Uh oh!

0xc0170 commented Apr 10, 2017

Uh oh!

pan- commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mazimkhan commented Apr 18, 2017

Uh oh!

sg- commented Apr 19, 2017

Uh oh!

mbed-bot commented Apr 19, 2017

Output

Uh oh!

adbridge commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented May 2, 2017

Uh oh!

Uh oh!

mazimkhan Mar 30, 2017 •

edited

Loading

pan- commented Apr 13, 2017 •

edited

Loading

adbridge commented May 2, 2017 •

edited

Loading