Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: Fix output values reported by PcpPrimIndex::PrintStatistics() #1480

Conversation

HamedSabri-adsk
Copy link

Description of Change(s)

This PR fixes the values displayed incorrectly when PcpPrimIndex::PrintStatistics() routine is called on Windows.

According to Linux manual page for printf

      '      For decimal conversion (i, d, u, f, F, g, G) the output is
              to be grouped with thousands' grouping characters if the
              locale information indicates any.  (See setlocale(3).)
              Note that many versions of gcc(1) cannot parse this option
              and will issue a warning.  (SUSv2 did not include %'F, but
              SUSv3 added it.)

https://man7.org/linux/man-pages/man3/printf.3.html

There is no such flag for Windows:

https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-160

hence the output values show up wrong on Windows and correct on Linux:

Windows:
windows_statistic_printf

Linux:
linux_statistic_printf

After the Fix:

fix_me

@HamedSabri-adsk
Copy link
Author

Hi @spiffmon, here is the fix that I mentioned earlier in our chat on google interest group yesterday.

Cheers,
H

@jtran56
Copy link

jtran56 commented Mar 22, 2021

Filed as internal issue #USD-6617

@c64kernal
Copy link
Contributor

Thanks for the fix @HamedSabri-adsk! We generally try very hard to avoid any platform-specific ifdefs outside of arch. Would it be possible to just remove offending "''" and instead do something a little more standard across all platforms but still visually easy to compare (e.g., everything right justified)? Thanks again!

@c64kernal c64kernal added the blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed label Apr 9, 2021
@HamedSabri-adsk
Copy link
Author

Hi @c64kernal,

sure, that's a good point. I removed the offending "'" all together. As for the text Alignment, I didn't want to make a bigger change than it is necessary since the alignment looks visually nice and readable even with a very large number (e.g 15 digits). Hope this Ok?

Addressed: 76705d8

  Total nodes:                       922337200000000
  Total culled* nodes:               922337200000000
  By type (total / culled*):         
    root:                            922337200000000 / 922337200000000
    inherit:                         922337200000000 / 922337200000000
      implied inherits:              922337200000000 / 922337200000000
    variant:                         922337200000000 / 922337200000000
    relocate:                        922337200000000 / 922337200000000
    reference:                       922337200000000 / 922337200000000
    payload:                         922337200000000 / 922337200000000
    specialize:                      922337200000000 / 922337200000000
  (*) This does not include culled nodes that were erased from the graph

@c64kernal
Copy link
Contributor

This seems totally reasonable to me, thanks so much @HamedSabri-adsk! We'll try to get it merged soon.

@c64kernal c64kernal added pending push and removed blocked Issue fix or pull request blocked until questions are answered or pending notes are addressed labels Apr 14, 2021
@pixar-oss pixar-oss merged commit 63b2fcc into PixarAnimationStudios:dev Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants