Skip to content

Conversation

@StephanTLavavej
Copy link
Member

  • Consistently follow our convention of defining variables on separate lines.
  • Simplify FBITS logic - it's always 24 (single-precision) or 53 (double-precision) for us.
    • This is now consistent with:

      STL/stl/src/xxxdtent.hpp

      Lines 18 to 42 in 484fbc9

      #if FBITS == 53
      #define FRAC_BITS 67108864.0L // 2^26
      static const FTYPE tenth[] = {
      // 53-bit: 0.100000
      static_cast<FTYPE>(FLIT(6710886.0) / FRAC_BITS),
      static_cast<FTYPE>(FLIT(26843545.0) / FRAC_BITS_2),
      static_cast<FTYPE>(FLIT(40265318.0) / FRAC_BITS_2 / FRAC_BITS),
      static_cast<FTYPE>(FLIT(26843545.0) / FRAC_BITS_2 / FRAC_BITS_2),
      };
      #elif FBITS == 24
      #define FRAC_BITS 4096.0L // 2^12
      static const FTYPE tenth[] = {
      // 24-bit: 0.100000
      static_cast<FTYPE>(FLIT(409.0) / FRAC_BITS),
      static_cast<FTYPE>(FLIT(2457.0) / FRAC_BITS_2),
      static_cast<FTYPE>(FLIT(2457.0) / FRAC_BITS_2 / FRAC_BITS),
      static_cast<FTYPE>(FLIT(2457.0) / FRAC_BITS_2 / FRAC_BITS_2),
      };
      #else // FBITS
      #error Unexpected value for FBITS
      #endif // FBITS
  • Wrap a slightly-too-long line in provision-image.ps1.
    • I verified that the script still succeeds.
  • Improve the README and avoid long lines.
    • Add links to our Status Chart and Discord at the very top, where they'll be noticed.
    • Organize them in a bulleted list below the Changelog, for even more prominence.
    • Slightly rephrase the IDE instructions to make them easier to scan (and avoid a long line). Drop "to install the boost-math dependency" as this is apparent from the command and the previous explanation.
    • Moderately reorganize the command prompt instructions. (I verified that they can be followed from scratch.)
      • Talking about changing the x64-windows constant was confusing - we're already depicting boost-math being installed for both x86-windows and x64-windows. What we really mean is to open up a different kind of Native Tools Command Prompt, and adjust the build directory accordingly. These instructions are so short, we can just depict them directly, avoiding all "you know what we mean" ambiguity.
      • The initial "one time" steps of cloning the repo and building vcpkg don't need a VS command prompt; rephrase the step to "Open a command prompt." to indicate that it doesn't matter.
      • The command-line instructions don't need to say "Invoke". Just mention the command.
      • Again, "to install the boost-math dependency" wasn't providing useful information; strike it.
      • Drop the "wherever you want binaries" generalization. We specifically .gitignore the /out/ directory. Our instructions are simple enough for advanced users to understand what paths they can replace.
      • (Longer-term, I would like to restructure the README even more along these lines, offering default incantations to build, update the paths, and run the tests - this is a step in that direction.)
    • Depict C:\Users\username\Desktop to follow our usual policies. (I would actually prefer C:\Temp here, but users tend to think that that is somehow special. Maybe C:\Anywhere instead?)
    • Adjust the testing command line examples to stay within 120 columns, by extracting the current directory.
      • Note that there's a slight adjustment here, as we're increasingly harmonizing our depicted paths - change C:\STL to C:\Dev\STL which was used in previous examples.
  • Validate line length for .cmd, .md, .ps1, .py.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Aug 24, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 24, 2020 04:09
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just suggestions, no actual changes requested.

@StephanTLavavej
Copy link
Member Author

@mnatsuhara Thanks - I've applied your suggestions!

@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2020
@StephanTLavavej StephanTLavavej merged commit c9a43aa into microsoft:master Aug 26, 2020
@StephanTLavavej StephanTLavavej deleted the cleanups branch August 26, 2020 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants