Skip to content
This repository was archived by the owner on Jun 5, 2019. It is now read-only.

add casts to suppress compiler warning CLR/Tools/Parser/ByteCodeParser.cpp line 389 and 390 #280

Open
doingnz opened this issue Aug 10, 2015 · 6 comments
Labels

Comments

@doingnz
Copy link
Contributor

doingnz commented Aug 10, 2015

Vs2015 compiler raises a warning for the following lines in CLR/Tools/Parser/ByteCodeParser.cpp as it is expecting float/double.

389 -    if(ref.m_ol->m_flags & CLR_RT_OpcodeLookup::ATTRIB_HAS_R4   ) wprintf( L" %f"         , ref.m_arg_R4  );  
390  -    if(ref.m_ol->m_flags & CLR_RT_OpcodeLookup::ATTRIB_HAS_R8   ) wprintf( L" %g"         , ref.m_arg_R8  );  

Adding the cast avoids the warning.

389 +    if(ref.m_ol->m_flags & CLR_RT_OpcodeLookup::ATTRIB_HAS_R4   ) wprintf( L" %f"         , (float)ref.m_arg_R4  );  
 390 +    if(ref.m_ol->m_flags & CLR_RT_OpcodeLookup::ATTRIB_HAS_R8   ) wprintf( L" %g"         , (double)ref.m_arg_R8  );  

The "ByteCodeParserCast" branch was made against a version of dev in doingnz repository that already includes DS5 support. I am a newbie to Git and don't know how to make it relative to a branch that matches the current dev in the main repository. As the edits are simple and for a single file, will leave that to a more experienced user :-)

@cw2
Copy link
Contributor

cw2 commented Aug 11, 2015

Duplicate of #257.

If I remember it correctly, those integer types are used in case the platform does not have support for floating point numbers - see usage of *_EMULATED_FLOATINGPOINT preprocessor symbols. So, I would probably wrap (float) and (double) casts in an appropriate #ifdef block, something like

#if !defined(TINYCLR_EMULATED_FLOATINGPOINT)
  ..., (float)ref.m_arg_R4);
  ..., (double)ref.m_arg_R8);
#else
... <the original code>
#endif

(?)

@doingnz
Copy link
Contributor Author

doingnz commented Aug 11, 2015

Good point!

Would it be better to do a typedef for platform_float and platform_double and then use #if/#else/#endif to define appropriately?

@lt72
Copy link
Contributor

lt72 commented Aug 11, 2015

#ifdef is not necessary. That flag refers to the MSIL instruction encoding, e.g. see ECMA-335 standard docs, Common Language Infrastructure (CLI), Partition III, CIL Instruction Set, section 3.27, page 62. Note the instruction modifiers?
We do handle those instruction whether the FP is emulated or not, the cast is always appropriate.

@cw2
Copy link
Contributor

cw2 commented Aug 15, 2015

@doingnz Are you going to do a pull request with the fixes?

@doingnz
Copy link
Contributor Author

doingnz commented Aug 16, 2015

I don't know how to make a pull request that isolates the change to that single file. (Git newbie. I like Git, but still learning!)

If I make a new pull request, then it includes all the differences between my dev and the NETMF dev. These changes will include adding support for DS5 build tools. (#273)

My changed file is here:

https://github.com/doingnz/netmf-interpreter/blob/0cee2a29b4418a49bf5556c0b0c97df7481728db/CLR/Tools/Parser/ByteCodeParser.cpp

@cw2
Copy link
Contributor

cw2 commented Aug 16, 2015

Well, I have looked at your repository - if I understand it correctly, you have all DS5-related changes in ARM_DS5 branch, but you've already merged it into your dev branch, right?

If I wanted to create a pull request with Visual C++ compiler warning fixes, I would first reset the dev branch - there are many ways how to do it, using TortoiseGit for example:

  1. Launch the log dialog ("Show Log"),
  2. Click the branch link in the top left corner,
  3. In the Browse Reference dialog select refs/remotes/upstream in the left tree, then dev in the right list (assuming the official NETM repo configured as upstream, as described on the wiki),
  4. Select the most recent commit in the list, marked with 'upstream/dev' (54fdabc)
  5. Right click and select 'Reset "dev" to this...'
  6. In the Reset dialog select Reset Type "Hard: Reset working tree and index..." (the last one),

Now the local dev branch is reset to the official remote version, you can create a new branch for ByteCodeParser fixes and make a pull request.

To synchronize local 'dev' branch to your git repo, use Push as usual, only this time select "known changes" in Force: May discard in the Options (i.e. force push). Note: This is the situation where you could cause some troubles to anyone who has already started working on their local repos based on your changes, but I guess you don't need to worry about it.

If you want to make it easier for the maintainers to merge your DS5 changes, or include ByteCodeParser fixes after they are merged in the official dev branch, you can rebase the ARM_DS5 branch onto dev - it is very similar to merge, except it does not create the merge commits - instead, rebase takes the status of dev branch and then applies your changes (commits) on top of it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants