• Greg (unregistered)

    Looks like they're allergic to references... That first loop would be a lot more efficient written as for(string const & name : invec) Thinking on it, maybe they don't know about references at all and that's the reason for the pointer in the map? That actually allows changing the value of the string pointed to, even if you're looping using copies of the vector content...

  • Artistic Reference (unregistered)

    Well Remi, you ARE right,

    But where's your sense of adventure ! Think about all the modern day explorers, crawling through this code base like cartographers trying to map the sources of Nile, choping their ways throught the jungle, the moskitos swarms of pointers to things yet to be discovered, the...

    And what a high level of creativity this is.

  • (nodebb)
    I won't pick on names here, as they're clearly anonymized.

    Remy, shouldn't you know better by now?

  • (nodebb)

    What about the WTF of them missing out the const reference for the invec parameter?

  • (author) in reply to Dragnslcr

    I had a little more context from the submitter, in this case that let me say that. ;)

  • (nodebb) in reply to Greg

    Looks like they're allergic to references... That first loop would be a lot more efficient written as for(string const & name : invec)

    Since 'invec' is copied/moved into 'func' and the standard associative containers own their keys, then a fairly optimal solution could be

    using stringy = std::unordered_map<std::string, std::string>; stringy func(std::vectorstd::string invec) { stringy umap{}; for(auto&& val : invec) { auto v = lookupValue(val); umap[std::move(val)] = std::move(v); }
    return umap; }

    The intermediate variable is there because I can't remember the happens-before rule in the case of 'stringy::operator = lookupValue(val);', and I don't want a use-after-move bug.

    Undoubtedly, that's a bit of cognitive load to reduce copying the strings, so TRWTF is using stringy types to begin with.

  • (nodebb)

    Is

    thingy.get<1>() = lookupValue(thingy.get<0>());

    missing a pointer dereference?

    *(thingy.get<1>()) = lookupValue(thingy.get<0>());

  • Marian (unregistered)

    I think about 15% of WTFs would go away if we all followed that rule.

    30%, by just iterating twice. I am sure.

  • sudgy (unregistered)

    There are way more WTFs here than were mentioned:

    • As mentioned above, not using references in the loops
    • Using .get<> instead of .first and .second
    • Most importantly, they are setting the pointer thingy.get<1>() itself, not what it's pointing to, making this operation a no-op, especially since thingy was copied by value in the loop, and the value written goes out of scope at the end of the current iteration of the loop.
  • mihi (unregistered)

    As a funny coincidence, at work today I implemented code that itereated over a list of values twice. First to compute their sum, then to normalize their values based on the sum. I am pretty sure I cannot merge those into one iteration.

  • (nodebb)

    So what happens if the input vector is large enough that umap needs to be resized while adding all the keys? Am I right in understanding that all those lovely pointers are now useless?

  • (nodebb)

    C++-style casts - an ugly notation for an ugly operation —Stroustrop, most likely.

  • Greg (unregistered) in reply to prueg

    According tp the unordered_map documentation, iterators will be invalidated when a rehash is needed, so it looks like this is may be implementation dependent. When that happens, all bets are off w.r.t. what happens when attempting to access the memory the pointer points to.

  • Greg (unregistered) in reply to thaines.astro

    Very likely. Since they're not using a reference in their ranged based loop, they're modifying a copy of the pointer to the string which will be a nop

  • Officer Johnny Holzkopf (unregistered)

    The line "vector<pair<string, string*> idxvec;" seems to be missing a closing ">", or is this just compensation for too much looping? I can't tell, I'm not a robot!

  • LiKao (unregistered) in reply to mihi

    In lazy functional languages you can normalize a set of values with (kind of) only one iteration. You go through them and divide every value by the lazy yet to be computed sum of all values and store the yet to be computed result in an array. Upon first access the actual normalized value will be computed.

    If you only access very few of those values, it might save a bit because only those values cause a division to happen (and add a bit for handling all the lazyness). But I doubt anyone outside of the Haskell crowd would ever dream up such a monstrosity.

    Our compiler Lecturer at University was a profound Haskell developer and showed us that this is in fact possible, but I don't remember enough Haskell to actually implement it.

Leave a comment on “A Pair of Loops”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article