Re: [patch] Implement ISO/IEC TS 18822 C++ File system TS

[Jonathan Wakely <jwakely at redhat dot com>]

On 01/05/15 21:28 +0200, Daniel Krügler wrote:
> 2015-05-01 20:22 GMT+02:00 Jonathan Wakely <jwakely@redhat.com>:
> > On 01/05/15 19:03 +0200, Daniel Krügler wrote:
> > > b/libstdc++-v3/src/filesystem/path.cc:
> > >
> > > - path::compare(const path& p) const noexcept:
> > >
> > > Shouldn't the implementation of this noexcept function not try to
> > > create copies of path objects? Couldn't _Cmpt just hold references to
> > > _M_pathname?
> >
> > All your other comments are definitely correct and I'll make the
> > relevant fixes soon, but I'm not sure what you mean here, could you
> > clarify?
>
> My remembrance of the difference in noexcept qualifications of
>
> int  compare(const path& p) const noexcept;
> int  compare(const string_type& s) const;
> int  compare(const value_type* s) const;
>
> is that the latter are allowed to allocate memory to be implemented as
> the standard writes the corresponding functions for std::basic_string:
>
> int compare(const basic_string& str) const noexcept;
> int compare(const charT* s) const;
>
> Returns: compare(basic_string(s)).
>
> But if I read your implementation of path::compare(const path& p)
> correctly it *also* may allocate memory by copying _M_pathname into a
> _Cmpt object.

Yes, I agree that there's a bug here that could cause it to
std::terminate.

> I was wondering whether for this comparison there exists
> real need to *copy* _M_pathname (potentially exceeding the memory
> limits). Wouldn't it be possible to define a _CmptRef type that for
> the purpose of implementing compare(const path& p) just refers to
> references of the _M_pathname and therefore doesn't need to allocate
> any dynamic storage? (I should have spoken of a new type _CmptRef and
> not of your existing _Cmpt).


Ah, I see what you mean (I thought you were suggesting some
improvements to _Cmpt itself ... which might be possible so I'm glad
you made me think about it :-)

I think I wrote compare() like that because it was easier, and when I
first implemented this we had COW strings so it wouldn't throw when
copying. That isn't true now, so I need to change it.