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

Do not depend on global states on string manipulation #94

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

kmaehashi
Copy link
Contributor

@kmaehashi kmaehashi commented Feb 2, 2022

This PR changes the string manipulation to avoid depending on global states. Specifically, this fixes the issue that:

Reproducer:

#include <locale>
#include <iostream>
#include <iomanip>
#include <string>

#include "jitify.hpp"

class comma_numpunct : public std::numpunct<char>
{
public:
   comma_numpunct(char thousands_sep, const char* grouping)
      :m_thousands_sep(thousands_sep),
       m_grouping(grouping){}
protected:
   char do_thousands_sep() const{return m_thousands_sep;}
   std::string do_grouping() const {return m_grouping;}
private:
   char m_thousands_sep;
   std::string m_grouping;
};


int main() {
    // (1) print_with_line_numbers depend on previously-set cout state
    std::cout << std::setbase(16);
    jitify::detail::print_with_line_numbers("1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n");
    // -> The line numbers are shown in hex.

    // (2) include guard generation depends on previously-set global locale
    std::locale comma_locale(std::locale(), new comma_numpunct(',', "\03"));
    std::locale::global(comma_locale);

    // (Quote from Jitify's pragma_once code)
    std::stringstream ss;
    // ss.imbue(std::locale::classic());
    ss << std::uppercase << std::hex << std::setw(8) << std::setfill('0')
       << 123456789;
    std::string include_guard_name = "_JITIFY_INCLUDE_GUARD_"   ss.str()   "\n";
    std::cout << include_guard_name << std::endl;
    // -> Generates a broken include guard: "_JITIFY_INCLUDE_GUARD_7,5BC,D15"
}

Copy link
Collaborator

@maddyscientist maddyscientist left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @kmaehashi

@maddyscientist maddyscientist merged commit 4a37de0 into NVIDIA:master Feb 3, 2022
@kmaehashi kmaehashi deleted the avoid-global-stream-state branch February 4, 2022 07:44
benbarsdell added a commit that referenced this pull request Feb 10, 2022
- The locale defaults to a global setting, which can mess up the
  expected serialization (e.g., insert commas in integer values).
- See #94.
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.

None yet

2 participants