• (nodebb)

    Hmm.

    OK. I'm not really happy about ra as a variable name for a shared-pointer-to-Assignment, but it's not an interesting mistake. Maybe there's a good reason for calling it that beyond simply an abbreviation for "reference-to-Assignment" (which it isn't, which is why I'm not happy about it).

    What bothers me is the article's blatant dismissal of using Assignment::Ptr instead of boost::shared_ptr<Assignment>, but even more, its unconditional dismissal of Assignment::makeAssignment(...). After all, whether it's C++ before C++11 (using Boost) or C++11+ (using the STL), the library provides a function to construct a new shared pointer and return a pointer to it. It's called the_right_namespace::make_shared<T>(...) with the forwarding (to the constructor) done in a way that's appropriate to the C++-version in use (perfect forwarding in C++11+, hypertemplated overloads in C++<11).

    That means that the only arguments are about snake_case versus camelCase, and whether make_shared<Assignment>(...) is better or worse than makeAssignment(...). I'm actually opposed, on the grounds of DRY, to having the function be a static member function of Assignment, and would much prefer it to be a namespace level function outside the class. It's not like it has to be a friend or anything stupid like that - it's just a forwarder-to-constructor.

    But the whitespace is definitely toothgrindingly stupid.

  • (nodebb)

    Sure be nice if there was a pretty-formatter you could run over your codebase and a source control diff engine that could intelligently ignore syntactically insignificant whitespace changes.

    Addendum 2025-02-05 09:30: I'd also like complete buy-in from my entire dev team on which format standard we use going forward.

    I also want a pony.

  • (nodebb) in reply to WTFGuy

    Sure be nice if there was a pretty-formatter you could run over your codebase and a source control diff engine that could intelligently ignore syntactically insignificant whitespace changes.

    Addendum 2025-02-05 09:30: I'd also like complete buy-in from my entire dev team on which format standard we use going forward.

    I also want a pony.

    My three wishes are SO different than yours!

  • (not a)[email protected](or is it) (unregistered) in reply to WTFGuy

    If you're the one controlling either the check in proces or the CI/CD pipeline you could just... I don't know... run said formatter in the CI/CD pipeline and commit back to source control with a format standard that's either your preferred style or most similar to what's currently in use. Might not make you popular, so it's best to ensure the commits with formatted (fixed) syntax use the identity of the CI/CD pipeline.

  • (nodebb)

    I'm not sure if this is actually THAT bad... researchers benefit a lot from DSL-like syntax, and the only thing they care about is their research, however the computer carries it out eventually.

  • (nodebb) in reply to Jay Vercellone

    Yes, but no. It sure as egcs is egcs would be nice for them to think about how they will continue working on the project over its whole lifetime rather than just slapping the code together and hoping it won't get in their way later.

  • OldCoder (unregistered)

    Having worked with some people like this a (very) long time ago, I suspect that the renaming is probably something simple like, they couldn't easily find the "_" on their keyboards.

  • (nodebb) in reply to Steve_The_Cynic

    Author, here.

    OK. I'm not really happy about ra as a variable name for a shared-pointer-to-Assignment, but it's not an interesting mistake. Maybe there's a good reason for calling it that beyond simply an abbreviation for "reference-to-Assignment" (which it isn't, which is why I'm not happy about it).

    In this instance, the variable name is actually tied to the surrounding context. In fact, I think now I should have included that context because it also demonstrates a complete lack of understanding of class invariants. These are sequential source lines.

    someType RS = ...; Assignment::Ptr rs = ... rs->addInput(RS);

    someType RA = ...; Assignment::Ptr ra = ... ra->addInput(RS);

    That second 'RS' isn't my typo. I think it's a copy/paste error from the creation of 'rs'. Why RS isn't a constructor parameter is another 15-year-old mystery.

    What bothers me is the article's blatant dismissal of using Assignment::Ptr instead of boost::shared_ptr<Assignment>, but even more, its unconditional dismissal of Assignment::makeAssignment(...).

    That part was editorialized (Bad, Remy, bad!). My problem with TYPE::Ptr everywhere is that the code no longer uses domain-specific vocabulary; instead revealing the inner workings to everyone and making it effectively impossibly to change them.

    That means that the only arguments are about snake_case versus camelCase

    no_comment

    I'm actually opposed, on the grounds of DRY, to having the function be a static member function of Assignment, and would much prefer it to be a namespace level function outside the class.

    That would be fine. I think std::make_unique and friends are a usable design. I'm speculating, but many parts of this code feel like they were (re?)written after someone read a chapter out of the GoF. In this case, they tried to implement a factory, but missed the mark. They are everywhere, too, with a zoo of names like 'makeFoo', 'instance', and 'create'. There are also lots of 'visitors' that don't visit.

  • (nodebb) in reply to Jay Vercellone

    I'm not sure if this is actually THAT bad... researchers benefit a lot from DSL-like syntax, and the only thing they care about is their research, however the computer carries it out eventually.

    Yes, but it's not DSL-like syntax. It's implementation details leaking into the names.

    pImpl? What's that? -- these devs 15 years ago

  • DML (unregistered) in reply to Steve_The_Cynic

    Two things.

    First, I don't see a problem with the Assignment::Ptr typedef. If it's a convention used throughout your code base (i.e. Anything::Ptr is a shared_ptr of some sort), it's not a big deal and does make things look a bit less cluttered. Plus, if you ever decide to switch from boost::shared_ptr to std::shared_ptr, you just need to update the typedef. Admittedly, this is more a matter of aesthetics than anything else. Especially if you want to do something like std::vector<Assignment::Ptr>. Given that the code is using Boost and not std shared_ptrs, the base probably predates C++11. Back then, this was not valid syntax: std::vector<boost::shared_ptr<Assignment>> -- you needed to put a space between the last two >, otherwise you'd get a compiler error. Either that or use a typedef for the pointer and not worry about it.

    Second, there is valid reason for Assignment::makeAssignment(), though who knows if it's in play here. If Assignment inherits from shared_from_this(either boost or std), then it's not safe to stack allocate it. You can only allocate a shared_ptr to it. If makeAssignment() is meant as a guard rail to require only shared_ptrs to be allocated, then it's justifiable. However, this code snippet doesn't give enough information to know if that's the case.

    However, given how pervasive the pattern seems to be in the code base, I do have my doubts that it's used as a guard rail as described above, but I could be wrong.

  • FTB (unregistered) in reply to DML

    "Plus, if you ever decide to switch from boost::shared_ptr to std::shared_ptr, you just need to update the typedef"

    This.

    Even if we ignore code refactoring; Using typedef to make an alias for things instead of typing them out longhand every time is a GOOD thing.

    I don't know if "Assignment::Ptr" is the neatest syntax but it is C++-like and I CERTAINLY don't want to see (or have to type) "boost::shared_ptr<Assignment>" every time a pointer is needed.

    (I'd personally go with "assignment_ptr")

    TLDR; typedef is your friend. Languages like Java and C# are much poorer for not having it.

  • (nodebb) in reply to DML

    For sure. In fact, you're agreeing-with-details with what I said. What I dislike about Assignment::makeAssignment is the DRY failure in its name - it repeats "Assignment" for no good reason. Either move it outside the class, as just makeAssignment or change its "final" name to be e.g. Assignment::create or Assignment::make. (I prefer "create", but whatever.) The function itself is just fine, if it's named correctly.

  • (nodebb) in reply to thaines.astro

    The value of the pImpl pattern hasn't gone away just because it's 2025 instead of 2010. (But the costs haven't gone away either...)

  • (nodebb) in reply to thaines.astro

    @thaines.astro Thanks for the answer, that was interesting. As an observation, Remy's suggestion of just leaving the naked boost::shared_ptr<Machin> everywhere would make the "revealed implementation details" thing even worse, because at least as it is, with Bidule::Ptr all over everywhere, the fully-detailed details (e.g. "it's a shared pointer" vs. "it's a unique pointer") aren't visible. And it's easier to refactor to be e.g. std::shared_ptr<Truc> or boost::unique_ptr<Truc> without special tools. (OK, the tools aren't that special, but why not use the language to help you?)

    Observation 2: if I use words like "machin" (it's not a mis-typing of "machine"...) or "bidule" or "truc", it's just a consequence of spending 16 years programming in a French-speaking French company in France (with the only relief from Frenchness being the occasional Francophone Belgian), and you should just think of them in terms of "thingie", "whatnot" or "wossname"...

  • (nodebb) in reply to FTB

    TLDR; typedef is your friend. Languages like Java and C# are much poorer for not having it.

    typedef is a bit Olde Skoole these days. (Our coding rules at $JOB tell us to use using instead. I'm not convinced that actually improves anything, but...)

  • DoContra (unregistered)

    This looks remarkably similar (story and WTF) to a codebase I have to deploy/help other PhD (student)s deploy into their dev machines. In that codebase, that particular typedef was there because they had rolled their own "smart" pointer and afterwards switched to boost (with the dead code still very much in the repo). They did use that pattern for iterators of collections of their custom types, but that is slowly being replaced with "auto".

  • DML (unregistered) in reply to Steve_The_Cynic

    Ah, ok... Kinda hard to notice in writing sometimes. I do kind of like using makeShared for those "make" functions to mirror the std/Boost versions, but your conventions are also acceptable.

  • Jimbob (unregistered)

    I dunno, isn't it normal practice for blocks to be indented more than the surrounding lines?

  • Christopher Jefferson (github)

    Those kind of 'boost-wrapping' typedefs were very common back around 2000 (and academic code often lives forever).

    The problem was back there things like shared_ptr moved from boost, to TR1 (a C++ extension), then into C++ (I think 11?), but then because this is academic code, you'd often want to be able to easily typedef your way back to the boost version for people with compilers that weren't yet on C++11.

    Why not just always use boost? Because boost was a pain in the ass to install, so if once everything you needed was in standard C++, it was nice to be able to just use stuff from the compiler. Fun times.

    That white-space looks to me a lot like it could be a disagreement on if tabs are 2, 4 or 8 spaces. I wonder if the original had any tabs in it?

  • (nodebb) in reply to Jimbob

    I dunno, isn't it normal practice for blocks to be indented more than the surrounding lines?

    Sure, but 40 columns?

  • Craig (unregistered) in reply to Christopher Jefferson

    As pasted into the story, the short indents are tabbed and the long indent is spaced. I would agree that it's almost certainly a hodge-podge of different tab widths (and tab vs space practice) that infected changes that were all trying to use a common indent level.

Leave a comment on “Whitespace: A Frontier”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article