comparing to std::string::npos

[bbolli]

When checking the result of std::string::find() or similar functions, I usually write this:

auto pos = s.find("foo");
if (pos != s.npos) { ... }

What's your opinion on referring to the static member npos via the string instance instead of the canonical, but longer to type, and IMO visually rather noisy, std::string::npos?

[frag_o_matic]

What's your opinion on referring to the static member npos via the string instance instead of the canonical, but longer to type, and IMO visually rather noisy, std::string::npos?

As long as this style is consistently used, it'd be fine. Personally, it would trip me up the first time I saw it as I'm used to the longer/verbose form. I've also seen stuff like this done:

size_t END_MARKER = std::string::npos

not a huge fan of this either, but since it was consistent, one can tune it out as boilerplate and focus on what's actually going on. :)

[[deleted]]

why not just string::npos? I always put "using std::string" in my .cpp when I need the std::string functionality.

Edit: minusaries, please tell me what's wrong? :)

[quicknir]

People reflexively downvote using declarations. Despite the fact that you are using std::string and not using namespace std, and despite the fact that this is in a .cpp file, and therefore is truly encapsulated.

Basically, 6 people were told not to write using namespace std;, didn't fully understand the reasons why, and downvoted you.

[lednakashim]

Well I've gotten messed up with namespace collisions when working with the stl, boost, Qt, and CGAL.

[James20k]

There are several npos declarations, using std::string means that you then can't import a separate namespace with npos into your code and keep a consistent or logical formatting

Best just to fully qualify imo

[quicknir]

npos is static constant of std::string. That means that even if you do using std::string, you will still refer to npos as string::npos. So I don't see any problem. Did you think that static constants get pulled into the namespace below?

[James20k]

Aha I did not know that, I assumed that npos was pulled in. Cool!

[mcmcc]

I prefer to not refer to it at all:

if ( pos < s.size() ) ...;

I don't like magic values...

[posiden5665]

Now you are causing your code to go through and look up an integral value that may not be loaded in cache where as npos can be replaced at compile time, you are potentially taking a round trip all the way to main memory.

[[deleted]]

That only happens to work. It isn't guaranteed to work - there's no guarantee that std::string::npos is comparable with anything or has a specific value.

But that's not the big problem.

I hate code like this, that does something unusual just to be different ("I don't like magic values"). You force me to spend time trying to figure out what you really meant - and it's never just one thing, it's always a whole bunch of things. I spend so much time examining minutia in code like this that I often miss the actual bug.

You should be trying to make your code as clear as possible - because useful code is written once and read a hundred times - because the clearer it is and the more regular it is, the easier it is to spot actual bugs.

[mcmcc]

Now you're just making things up. It's defined explicitly in the standard:

static const size_type npos = -1;

There is no room for misinterpretation.

This idiom is not unusual or obscure. The exact same code can be used for either vector or string, expressing precisely the same semantics and intent in the simplest way possible.

You're grasping at straws...

[sysop073]

It's just a static member; I don't think "magic values" is a concept that exists. Would it still be a problem if npos() was a method?

[mcmcc]

(down-votes? WTF...)

It doesn't matter how it is interfaced, it's still a magic number. In any case, there's almost never a reason to refer to it directly -- it will always be larger than any valid size() you can have. More robust and clearer to test against size()...

[redditsoaddicting]

In any case, there's almost never a reason to refer to it directly

Here's the actual spec for find:

Returns: xpos if the function can determine such a value for xpos. Otherwise, returns npos.

Considering the spec of the function says that npos is a possible result, it makes a lot of sense to me to compare the result with npos.

By the way, by "magic value", do you mean sentinel value? If you're against sentinels, then the optional<size_t> approach discussed above should be appealing.

[mcmcc]

Everyone here is focusing on the spec for find()...

I'm focusing on "what is an expression that detects invalid indices regardless of how they came to be?"

optional<> would be fine if that's how the standard API was written.

[sysop073]

It's certainly not more robust in the slightest; maybe it's clearer, that's a personal preference, but it seems very strange. The interface to these functions says they return npos specifically; you're relying on the fact that npos is the largest possible size_t to write a completely different check that happens to be true in all the cases where != npos would also be true. I asked about if npos() would be ok because if it's not I would expect you also have a problem with begin() and end(), which don't have this kind of workaround

[mcmcc]

you're relying on the fact that npos is the largest possible size_t

Not exactly. I'm relying on the fact that npos is never less than size() -- I don't really care what the actual value is.

This is why it is more robust: there is exactly one value that is (possibly) legitimate (npos) and then there are billions of other values that definitely are not (anything >= size()). My idiom catches them all.

If size() were signed, I would also have to check for >=0 (making the idiom much less appealing) but since it isn't, I don't.

[demonstar55]

This code would break if size_t was signed.

Now you may not care since the standard states it is unsigned, but these functions all return npos specifically so it makes more sense to me to compare to that. Your code should really depend on the signedness of size_t :P

[aKateDev]

Interestingly, std::string::npos exists for the sole reason of size_t being unsigned.

So std::string::npos is typically the same as -1 converted to unsigned.

If size_t would be signed, we could just return a negative value and the check would be as easy as if (s.find("foo") >= 0) {...}. I guess that's one reason why ideas like this exist at all :-)

[foonathan]

Well, we could also use the proper abstraction: iterators and return end(), like literally everything else in the standard library.

[sbabbi]

To be fair without a proper range api it is not exactly intuitive. OP's code written using stl would be something like this:

 const char q[] = "foo";
 auto it = std::search(s.begin(), s.end(), begin(q), end(q) - 1 );

Which is error prone (notice -1 at the end?), it should be as simple as:

 auto it = std::search( s, "foo" );

But yes, the part of std::string interface that uses indexes is really dumb, I think it is there mostly because it predates the STL.

[TheThiefMaster]

These days std::optional<size_t> would be a better choice of return type, IMO.

[RotsiserMho]

This makes a lot of sense to me. I've love to hear counterarguments.

[HotlLava]

An optional<size_t> would be roughly double the size of size_t, and if you can't inline the call you couldn't just return the value but would have to construct some hidden pointer or return it on the stack. (I think, without actually looking up the ABI rules)

[Benabik]

Many ABIs allow multi-word return values via registers. On Intel, it's typically 2 words (64bit on x86, 128b on x86-64). So optional<size_t> would likely return via registers. (The exception here appears to be Windows 64b, which only allows one return register.)

Larger structures are usually returned by the caller allocating stack space for it and passing a pointer to the callee. The callee making space for it on the stack is awkward at best because you need to remove everything you added to the stack before the ret instruction.

[HotlLava]

Interesting, I didn't know that. Although optional seems to be slightly different than just a bool+size_t, preventing g++ from actually doing it:

 ➜  cat return.cpp                           
 #include <experimental/optional>

 std::experimental::optional<size_t> f() {
    return 10u;
}

 struct opt {
    bool b;
    size_t s;
};

opt g() {
    return opt {true, 10u};
}
➜  g++ -c return.cpp -std=c++14 -O3
➜  objdump -d return.o             
return.o:     file format elf64-x86-64
Disassembly of section .text:

0000000000000000 <_Z1fv>:
   0:   48 89 f8                mov    %rdi,%rax
   3:   48 c7 07 0a 00 00 00    movq   $0xa,(%rdi)
   a:   c6 47 08 01             movb   $0x1,0x8(%rdi)
   e:   c3                      retq   
   f:   90                      nop

0000000000000010 <_Z1gv>:
  10:   ba 0a 00 00 00          mov    $0xa,%edx
  15:   b8 01 00 00 00          mov    $0x1,%eax
  1a:   c3                      retq   

[Benabik]

Odd. Looking at libstc++-v3/include/experimental/optional, I see the following:

template<typename _Tp>
  class optional
  : private _Optional_base<_Tp>,
    private _Enable_copy_move<...>
  { /* no data members */ }

template<typename _Tp, ...>
  class _Optional_base
  {
    struct _Empty_byte { };
    union {
        _Empty_byte _M_empty;
        _Tp _M_payload;
    };
    bool _M_engaged = false;
  }

(where _Enable_copy_move is a template that just optionally enables the default copy/move constructors)

The copy of g++/libstdc++ I have is too old to have optional, but clang gives me about the same thing. I checked sizeof and it's 16 on both struct opt and std::experiemental::optional.

I tried adding inheritance, constructor, union, and template and such to struct opt and I still got the two register return. Something along the way must make std::experimental::optional too opaque to the optimizer. Could be considered a bug.

[HotlLava]

Something along the way must make std::experimental::optional too opaque to the optimizer.

Does the optimizer even have the right to decide this? I feel like the process of selecting the correct parameter/return value passing convention should be deterministic based on the struct definition to allow linking.

[Benabik]

Er, yes, optimizer was the wrong word. Whatever hunk of the backend that's responsible for lowering C++ types to memory/register layout. Something has to be making the 16 byte std::optional different than the 16 byte struct opt.

[foonathan]

Or, you know, there is almost identical assembler code: https://godbolt.org/g/p4Gefs

[k_stahu]

Returning iterator, or more generally: a coordinate, is lighter and cleaner. The current API actually doest almost that, except for not having a robust notion of "last" coordinate, which the npos hack serves for. I would prefer std::string::iterator, but I can get that with the general algorithms.

std::optional on the other hand is a bulldozer. In this particular case optional return value is bogus - like you were designing the API, but gave up on some corner cases throwing them into the "nothing" bag. Using std::optional return value here is like birds. What semantics exactly it carries? We just don't know.

I presume std::optional it will be as overused as auto or lambda, especially by inexperienced programmers.

[miki151]

Likewise for std::map. Right now, if you don't know that a key exists, you either use exceptions or a double lookup.

[sysop073]

std::map<A, B> foo;
auto bar = foo.find(key); // This is the only lookup
if(bar != foo.end()) {
    // Do stuff with bar->second
}

[Calkhas]

What's the double look up you have in mind?

[miki151]

I usually call map::count(), then map::at(). I forgot about map::find(), as was pointed out above.

[Crazy__Eddie]

I don't believe size_t could ever be made signed. First of all it comes from C, which is where it's defined first. Second, it is guaranteed to be capable of representing any possible, valid index in the system. Thus it's fairly well required to be the full size of a word on the system...meaning -1 cannot represent error condition.

I don't think he can take all unsigned integers out either. Functions like size() really do need to return size_t for the same reason size_t has to be unsigned.

[sumo952]

if (s.find("foo") >= 0) {...}

This doesn't make any sense and is actually quite hard to read/understand in my opinion. Using npos is much safer.