Given warm feedback in #109 (comment) I want to continue working on that, but would appreciate some guidance from Microsoft team of you intend to merge such PR.

I would also appreciate feedback on how would you like to approach this in context of #211.

My idea for that would be merge #212 (this) first, then rebase #211 on top, so there's no need for typedefing stuff. I will review and test #211 once I'm done here, but I may need a few more days due to other time constraints

This is great. The more standard types we can use, the better.

You replaced all the names, including those in comments, including those reflecting the type's intention. HRESULT isn't equivalent to int32_t -- these type names carry semantic meanings.

For example, the code is messed up because all HRESULT error code macro definitions in /src/CalcManager/Ratpack/CalcErr.h are replaced with uint32_t, whereas all usages of HRESULT in src/CalcManager/CalcException.h are replaced with int32_t. I don't know whether the code directly compared two differently typed HRESULTs (esp. using the SUCCEEDED macro), which would create a havoc.

Please bear in mind that you're not just coding in C++. You're coding in C++ with Win32 conventions, where WPARAM, HRESULT and the likes are Win32-standard types.

Other than those, I do find some methods, e.g., CCalcEngine::ProcessCommand using WPARAM, strange. I would imagine them using DWORD instead. It's worth careful inspection of what semantics those data types carry and why they are used in the first place (e.g., you might want to use WPARAM if it came from a window message, which doesn't seem to be the case now) before replacing them. Also, use refactor tools to replace data types. Not "Find & Replace".

I'm aware of the comments issue, I labeled the commit incomplete for that reason, as well as explained that in opening comment.

Win32 should not matter in the core/engine part of a calculator.

The intent of this PR is to not introduce behaviour changes, so any changes you suggest should be done in a separate one.

Given warm feedback in #109 (comment) I want to continue working on that, but would appreciate some guidance from Microsoft team of you intend to merge such PR.
I would also appreciate feedback on how would you like to approach this in context of #211.
My idea for that would be merge #212 (this) first, then rebase #211 on top, so there's no need for typedefing stuff. I will review and test #211 once I'm done here, but I may need a few more days due to other time constraints

@fwcd, how do you feel about @janisozaur's suggestion for merging both changes? It'll give each of you an opportunity to contribute. It seems like we should wait to merge until we have CI builds running with Clang so we don't regress later.

I plan to resume work on it this weekend, maybe a bit earlier.

@danbelcher-MSFT That sounds good.

I have rebased this now, but it's not fully done yet.

I've implemented all the things I wanted for this PR.

Here's a brief summary:

• I replaced all the non-standard types used in src/CalcManager
  • mostly based on https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types
• This affected signature of ULongAdd and ULongMult, which are not available in the C++ standard
  • I ported the relevant pieces of code from https://github.com/dotnet/coreclr/blob/8b1595b74c943b33fa794e63e440e6f4c9679478/src/pal/inc/rt/intsafe.h
  • Available under MIT License
  • I added explicit mention where the code comes from
  • Had to make some changes (types used, symbol names), but left it as verbatim as I could and I made changes in separate commit to make review of them easier
  • I hope that's fine with you, if it isn't, then alternatively https://github.com/DavidEGrayson/intsafe might be a viable option?
• min and max are as painful to use with MSVC as ever
  • There's NOMINMAX macro that should be enabled to make MSVC behave closer to the standard

With these changes I can successfully compile the code with GCC and clang (though only with libc++, see #321), the test suite also seems to be passing on Azure.

There are 16 commits in total for now, please let me know if you would like them rebased any other way.

It seems like we should wait to merge until we have CI builds running with Clang so we don't regress later.

My (obviously biased) advice is to not block this on having CI builds with clang, this is something that would be easier to restore if regressed than having to start over because of conflicts, etc. I've already left some comments on #211, there are still ones that need to be addressed. Once #212 (this) gets merged I'd expect @fwcd to update #211, which I will review for the final time and it could be merged.

I have little experience with Azure Pipelines, but will start looking into ways clang and GCC can be tested so it doesn't regress. I hope to come up with something within a week or two, if someone doesn't beat me to it.

@danbelcher-MSFT @fwcd does that sound like a possible way forward? I'd appreciate quick feedback, I will try to address it as soon as I can, so this PR doesn't get stale by other changes.

@danbelcher-MSFT @HowardWolosky is there anything here you still want changed?

Like @GeeLaw, I have concerns about if the updated types reflect the correct use of that data. For example, now there is code such as int32_t hr = S_OK and why am I passing a uintptr_t as an opcode? This change is great in that the size of the data remains consistent across architectures before and after the change. What I think this change is missing is that it drops some of the semantics for how this data is used. Typenames tell you not only the size of the data, but also how that data is intended to be used.

Is there a middleground approach? Something along the lines of:

#ifndef HRESULT
#define HRESULT int32_t
#endif

Or is this a no-go?

I see your point. How about I will typedef the replacements where I deem applicable (HRESULT example sounds fine, but I'd rather name it ResultCode or something along those lines; I'd rather leave LONG -> int32_t as it is now) for now and in future perhaps they could be replaced with enum class for proper type management?

Feel free to provide suggestions on how the names should map.

why am I passing a uintptr_t as an opcode?

This is what original code did. Perhaps that was not intentional, as was pointed out by @GeeLaw's comment:

I do find some methods, e.g., CCalcEngine::ProcessCommand using WPARAM, strange. I would imagine them using DWORD instead

Still, I'd rather typedef it to uintptr_t and leave changing that to another PR, so this does not alter any of the current behaviour. Does that sound like a plan?

Can you address other points I raised in #212 (comment)?

How about I will typedef the replacements where I deem applicable

Sounds good to me! Definitely in support here.

I'd rather leave LONG -> int32_t as it is now

Great, I think that's a perfectly valid change to make.

in future perhaps they could be replaced with enum class for proper type management?

Absolutely, this is an improvement we should all be in favor of.

This is what original code did... Still, I'd rather typedef it to uintptr_t and leave changing that to another PR, so this does not alter any of the current behaviour. Does that sound like a plan?

That's fair. For my previous comment, I'm not necessarily asking for any change here at all, just pointing out that we should be concerned about the semantics of the types we're using and asking you to take care in your changes to make sure the code is still readable and reflects intent. For cases like uintprt_t, I think we all agree it's an odd choice of parameter type. Who knows what development constraints were like back in 1995 :) Choosing a standard type that at least does not change the behavior of the code seems like the best approach for this PR until we can fix later.

Can you address other points I raised in #212 (comment)?

Sure. Sorry for the delay.

This affected signature of ULongAdd and ULongMult, which are not available in the C++ standard
I ported the relevant pieces of code from link

Thank you! for going through the effort of finding an appropriate replacement. The parts of the code that use this are all concerned with manual management of memory. I do wonder if the better change here is to replace with std C++ functionality, so we're safer and more portable.

min and max are as painful to use with MSVC as ever. There's NOMINMAX macro that should be enabled to make MSVC behave closer to the standard

Yep, this bugs me too. Let's disable the macros.

My (obviously biased) advice is to not block this on having CI builds with clang, this is something that would be easier to restore if regressed than having to start over because of conflicts, etc. I've already left some comments on #211, there are still ones that need to be addressed. Once #212 (this) gets merged I'd expect @fwcd to update #211, which I will review for the final time and it could be merged.

Okay, to recap, we're merging #212 (this) first, then #211 after. With 211, I had wanted to install Clang myself and try out the changes, but I haven't had time yet. We'll go ahead with merging those without CI support, which will be added at a later date. The community will need to help out by raising an issue if the build regresses, until we can stand up the new environment in our CI build.

Side note, if you have simple setup instructions for an engineer who has only developed with MSVC, I would be happy for the help :) Maybe this is something that should be added as documentation to help cross-pollinate developers coming from different platforms?

Did that address all your questions or was there something else?

I changed HRESULT and WPARAM, does it look fine now? I realise the casing on types is inconsistent, do you want it done any particular way?

min and max are as painful to use with MSVC as ever. There's NOMINMAX macro that should be enabled to make MSVC behave closer to the standard

Yep, this bugs me too. Let's disable the macros.

Can you submit PR for that?

This should have all the latest feedback incorporated. Let me know if there's anything more that requires updating.

if you have simple setup instructions for an engineer who has only developed with MSVC, I would be happy for the help

There are a few ways you can try.

1. Cygwin, a POSIX-compatible environment that runs natively on Microsoft Windows. You get the (NSIS-based) installer, which acts as a package manager, you select packages you wish to have installed and an environment will be populated with them.

• Cygwin acts as a full environment, I'd say it has most complete user-space implementation, so applications actually target Cygwin itself, not Windows.

2. MinGW/MSYS. A port of GCC toolchain and accompanying binutils for Windows. MinGW is the toolchain part, while MSYS is the distribution that has a bunch of packages.
3. Mingw-w64. A fork of original MinGW over some petty issues, I think it's the more popular one.

• MSYS2, a fork of MSYS, that uses pacman package manager.
• "MinGW distro"/"nuwen", maintained by STL himself

4. Clang, at least I think so.

• Clang on Windows is commonly used as a toolchain for MSVS, but that uses clang-cl which enables MS ABI and some extensions. I was never able to get clang-proper working on Windows with CMake (which Add CMake/Clang support for the Calculator Engine #211 introduces), but invoking it manually should work fine.

5. Docker

• There are images containing recent toolchains, e.g. https://hub.docker.com/_/gcc/

6. WSL

• https://www.microsoft.com/en-us/store/top-free/apps/pc?category=Developer+tools%5cServers

I think I would recommend MSYS2 (or its sibling nuwen), if you want to keep it relatively lightweight. Otherwise, consider WSL.

To first configure with CMake, you would do cmake <path_to_dir_with_CMakeLists.txt>, then cmake --build ., it's usual to do builds outside of source tree, i.e.

$ git clone https://github.com/Microsoft/calculator
<some git status here>
$ mkdir calc-build
$ cd calc-build
$ cmake ../calculator
-- The CXX compiler identification is GNU 8.2.1
-- Check for working CXX compiler: /usr/lib/ccache/bin/c++
-- Check for working CXX compiler: /usr/lib/ccache/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: calc-build
$ cmake --build . # same as `make` here
<some status messages>
[100%] Linking CXX static library libcalc.a
[100%] Built target calc
$

This is now updated with all the points addressed, CI is green too.

@janisozaur, thanks for this change! Thank you also for the instructions you provided above. I'll try to get set up and we can work on getting the Clang support checked-in as well.

Looks like this regressed our ARM and ARM64 builds: https://dev.azure.com/ms/calculator/_build/results?buildId=7838

@janisozaur, can you help investigate?

Sure, on it.