[libcxx] r273034 - Add Filesystem TS -- Complete

Mon Aug 8 17:27:15 PDT 2016

Ping!

On Thu, Aug 4, 2016 at 8:03 AM, Adrian Prantl <aprantl at apple.com> wrote:
>
>> On Aug 3, 2016, at 1:56 PM, Bruno Cardoso Lopes <bruno.cardoso at gmail.com> wrote:
>>
>> Hi Eric,
>>
>> After we upgraded our green dragon bots to El Captain (10.11), the
>> test below started to fail on some of our machines:
>>
>> -- test/std/experimental/filesystem/fs.op.funcs/fs.op.hard_lk_ct/hard_link_count.pass.cpp
>>
>>  TEST_CASE(hard_link_count_for_directory)
>>  {
>>      uintmax_t DirExpect = 3;
>>      uintmax_t Dir3Expect = 2;
>>  #if defined(__APPLE__)
>>      DirExpect += 2;
>>      Dir3Expect += 1;
>>  #endif
>
> Just as a general code review comment: When committing a platform-specific workaround, I would expect there to be a comment explaining what bug/peculiarity of the platform is being worked around :-)
>
> thanks,
> Adrian
>
>>      TEST_CHECK(hard_link_count(StaticEnv::Dir) == DirExpect);
>>      TEST_CHECK(hard_link_count(StaticEnv::Dir3) == Dir3Expect);
>>
>>      std::error_code ec;
>>      TEST_CHECK(hard_link_count(StaticEnv::Dir, ec) == DirExpect);
>>      TEST_CHECK(hard_link_count(StaticEnv::Dir3, ec) == Dir3Expect);
>>  }
>>
>> You can see the issue in every recent (past 10 days) run on
>> http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-expensive/433
>>
>> I noticed that commenting out the snippet around "#if
>> defined(__APPLE__)" would make it work. What's the rationale behind
>> the "#if defined(__APPLE__)" above?
>>
>> Thanks,
>>
>>
>> On Tue, Jun 21, 2016 at 3:03 PM, Eric Fiselier via cfe-commits
>> <cfe-commits at lists.llvm.org> wrote:
>>> The issue should be fixed in r273323.
>>>
>>> Thanks for the report and for your patience.
>>>
>>> /Eric
>>>
>>> On Mon, Jun 20, 2016 at 11:27 PM, Eric Fiselier <eric at efcs.ca> wrote:
>>>>
>>>> Hi Artem,
>>>>
>>>> Sorry for the delay, I've been busy with school all day.
>>>> I'll check in a fix tomorrow morning.
>>>>
>>>> Sorry again,
>>>>
>>>> /Eric
>>>>
>>>> On Mon, Jun 20, 2016 at 2:31 PM, Eric Fiselier <eric at efcs.ca> wrote:
>>>>>
>>>>> Oh shoot, I definitely didn't take that into account. I'll put together a
>>>>> fix.
>>>>>
>>>>> /Eric
>>>>>
>>>>>
>>>>>
>>>>> On Mon, Jun 20, 2016 at 2:27 PM, Artem Belevich <tra at google.com> wrote:
>>>>>>
>>>>>> Eric,
>>>>>>
>>>>>> Some tests appear to fail if the path to the tests' current directory
>>>>>> has some symlinks in it.
>>>>>> In my case source and build tree are in directory 'work' that's
>>>>>> symlinked to from my home directory:
>>>>>> /usr/local/home/tra/work -> /work/tra
>>>>>>
>>>>>> This causes multiple failures in libcxx tests. One example:
>>>>>>
>>>>>>
>>>>>> projects/libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.rename/rename.pass.cpp:121
>>>>>> 121        TEST_CHECK(read_symlink(bad_sym_dest) == dne);
>>>>>>
>>>>>> dne:
>>>>>>
>>>>>> /usr/local/home/tra/work/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/dne
>>>>>> bad_sym_dest:
>>>>>>
>>>>>> /usr/local/home/tra/work/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/bad_sym2
>>>>>>
>>>>>> These are the paths that traverse the 'work' symlink in my home
>>>>>> directory. However, the symlink that we're reading contains physical path to
>>>>>> the directory. While it does link to the right place, it's not the path the
>>>>>> test expects.
>>>>>>
>>>>>> #readlink
>>>>>> /usr/local/home/tra/work/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/bad_sym2
>>>>>>
>>>>>> /work/tra/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/dne
>>>>>>
>>>>>> I think we need to normalize paths before we compare them.
>>>>>>
>>>>>> --Artem
>>>>>>
>>>>>>
>>>>>> On Sat, Jun 18, 2016 at 12:03 PM, Eric Fiselier via cfe-commits
>>>>>> <cfe-commits at lists.llvm.org> wrote:
>>>>>>>
>>>>>>>> I assume the correct way to fix this is to disable
>>>>>>>> -Wcovered-switch-default while compiling
>>>>>>>> libcxx/src/experimental/filesystem/operations.cpp
>>>>>>>
>>>>>>> Agreed. Disabled in r273092.
>>>>>>>
>>>>>>> Thanks for your patience with this latest change,
>>>>>>>
>>>>>>> /Eric
>>>>>>>
>>>>>>> On Sat, Jun 18, 2016 at 12:54 PM, Adrian Prantl <aprantl at apple.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hello Eric,
>>>>>>>>
>>>>>>>> this commit causes new warnings on our bots:
>>>>>>>>
>>>>>>>> clang/src/projects/libcxx/include/fstream:816:5: warning: default
>>>>>>>> label in switch which covers all enumeration values
>>>>>>>> [-Wcovered-switch-default]
>>>>>>>>    default:
>>>>>>>>
>>>>>>>> The problem is with this defensive default statement in fstream:
>>>>>>>>
>>>>>>>>
>>>>>>>> template <class _CharT, class _Traits>
>>>>>>>> 0792 typename basic_filebuf<_CharT, _Traits>::pos_type
>>>>>>>> 0793 basic_filebuf<_CharT, _Traits>::seekoff(off_type __off,
>>>>>>>> ios_base::seekdir __way,
>>>>>>>> 0794                                         ios_base::openmode)
>>>>>>>> 0795 {
>>>>>>>> 0796 #ifndef _LIBCPP_NO_EXCEPTIONS
>>>>>>>> 0797     if (!__cv_)
>>>>>>>> 0798         throw bad_cast();
>>>>>>>> 0799 #endif
>>>>>>>> 0800     int __width = __cv_->encoding();
>>>>>>>> 0801     if (__file_ == 0 || (__width <= 0 && __off != 0) || sync())
>>>>>>>> 0802         return pos_type(off_type(-1));
>>>>>>>> 0803     // __width > 0 || __off == 0
>>>>>>>> 0804     int __whence;
>>>>>>>> 0805     switch (__way)
>>>>>>>> 0806     {
>>>>>>>> 0807     case ios_base::beg:
>>>>>>>> 0808         __whence = SEEK_SET;
>>>>>>>> 0809         break;
>>>>>>>> 0810     case ios_base::cur:
>>>>>>>> 0811         __whence = SEEK_CUR;
>>>>>>>> 0812         break;
>>>>>>>> 0813     case ios_base::end:
>>>>>>>> 0814         __whence = SEEK_END;
>>>>>>>> 0815         break;
>>>>>>>> 0816     default:
>>>>>>>> 0817         return pos_type(off_type(-1));
>>>>>>>> 0818     }
>>>>>>>>
>>>>>>>>
>>>>>>>> I assume the correct way to fix this is to disable
>>>>>>>> -Wcovered-switch-default while compiling
>>>>>>>> libcxx/src/experimental/filesystem/operations.cpp
>>>>>>>>
>>>>>>>> Could you please investigate?
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>> Adrian
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> cfe-commits mailing list
>>>>>>> cfe-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> --Artem Belevich
>>>>>
>>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>>
>> --
>> Bruno Cardoso Lopes
>> http://www.brunocardoso.cc
>

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc