Fix libclang's objc and objcpp support

[Chilledheart]

For c++ source, it is correct to use "-x c++" language options but it doesn't true for objective-c++ and objective-c source code. This patch will fix it.

Add three unittests, subcommands unittests will be added next time.

---

This change is 

[codecov[bot]]

Codecov Report

Merging #1624 (5e9433d) into master (6c9c089) will increase coverage by 0.17%. The diff coverage is 100.00%.

:exclamation: Current head 5e9433d differs from pull request most recent head 255c007. Consider uploading reports for the commit 255c007 to get more accurate results

@@            Coverage Diff             @@
##           master    #1624      +/-   ##
==========================================
+ Coverage   96.10%   96.28%   +0.17%     
==========================================
  Files          90       90              
  Lines        8035     7908     -127     
  Branches      164      164              
==========================================
- Hits         7722     7614     -108     
+ Misses        261      242      -19     
  Partials       52       52              

[puremourning]

Thanks for the PR.

In general when sending a PR to fix an issue it helps to explain what the problem is that's being fixed, and a set of steps that reproduce the problem (in much the same way as when raising an issue on the tracker). This makes it a lot easier to understand the motivation for the change and why it's necessary.

so in this case am I understanding correctly that the compiler invocation is clang++ <stuff> some_objective_c.m, and that ycmd is seeing the clang++ without a -x flag and assuming that it's c++ ?

[Chilledheart]

Thanks for the PR.

In general when sending a PR to fix an issue it helps to explain what the problem is that's being fixed, and a set of steps that reproduce the problem (in much the same way as when raising an issue on the tracker). This makes it a lot easier to understand the motivation for the change and why it's necessary.

so in this case am I understanding correctly that the compiler invocation is clang++ <stuff> some_objective_c.m, and that ycmd is seeing the clang++ without a -x flag and assuming that it's c++ ?

I haven't checked the source code of clang driver but it should depends on the suffix of source code filename if it isn't passed with "-x" flag.

[Chilledheart]

Reviewed 8 of 8 files at r1, all commit messages. Reviewable status: 0 of 2 LGTMs obtained (waiting on @Chilledheart)

ycmd/completers/cpp/flags.py, line 373 at r1 (raw file):

  if any( fl.endswith( '.mm' ) for fl in reversed( flags ) ):
    return [ first_flag, '-x', 'objective-c++' ] + flags[ 1: ]

perhaps instead we should return flags like we do for coda.

the reason is that then we let libclang decide based on the filetype, rather than trying to guess.

We're guessing for clang++ ... because that's the common use case.

It is better. I tested with the new code and it works.

ycmd/tests/clang/diagnostics_test.py, line 399 at r1 (raw file):

  @MacOnly

why Mac only? I mean, there's nothing stopping the obj c tests working on other OS is there? I realise that it is unlikely objc is used on other platforms.

Not tested on a non-Mac Os for objective-c support and I am not sure if libclang works for objective-c there.

[Chilledheart]

Thanks for the pull request. I did leave two small comments, but the change to flags.py looks fine.

Reviewed 6 of 8 files at r1, 2 of 2 files at r2, all commit messages. Reviewable status: 0 of 2 LGTMs obtained (waiting on @Chilledheart and @puremourning)

ycmd/tests/clang/diagnostics_test.py, line 399 at r1 (raw file):

Previously, Chilledheart wrote… I agree with @puremourning. If the tests pass on linux/windows we shouldn't use @MacOnly.

It seems to work on platforms other than mac. Let me remove it. https://github.com/Chilledheart/ycmd/runs/4727423777?check_suite_focus=true

ycmd/tests/clang/flags_test.py, line 66 at r2 (raw file):

  for to_remove in to_removes:
    assert_that( [ compiler ] + language_flag + [ '-c', filename ] + expected,

This seems odd. flags.py intentionally removes -c filename part, as leaving it in would cause errors during compiler invocation.

No, it is still there in flags otherwise you cannot tell if it is a cuda/objc/objcpp file in flags.py

[Chilledheart]

Reviewed 1 of 1 files at r3, all commit messages. Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/tests/clang/diagnostics_test.py, line 399 at r1 (raw file):

Previously, Chilledheart wrote… I still see @MacOnly decorators in this file. I'm guessing that was an oversight.

Done.

ycmd/tests/clang/flags_test.py, line 66 at r2 (raw file):

Previously, Chilledheart wrote… https://github.com/ycm-core/ycmd/blob/master/ycmd/completers/cpp/flags.py#L43

https://github.com/ycm-core/ycmd/blob/master/ycmd/completers/cpp/flags.py#L420

But that happens well after _AddLanguageFlagWhenAppropriate().

I see. I thought "-c" flag was a require_argument option for gcc/clang until I looked at the source code of clang: https://github.com/llvm-mirror/clang/blob/master/include/clang/Driver/Options.td#L549

Let me remove the skippable "-c" flag.

[Chilledheart]

Then I noticed the fact that RemoveUnusedFlags whose caller is PrepareFlagsForClang which is only called when it comes to a compiler database it is not true for the unittest where only _RemoveFlagsPrecedingCompiler is called.

https://github.com/ycm-core/ycmd/blob/master/ycmd/completers/cpp/flags.py#L420 https://github.com/ycm-core/ycmd/blob/master/ycmd/completers/cpp/flags.py#L281 https://github.com/ycm-core/ycmd/blob/master/ycmd/completers/cpp/flags.py#L331

Another issue is that if I remove "-c" in the unittests they fail because the method _RemoveFlagsPrecedingCompiler will assume the first flag, i.e. filename should be skipped.

[Chilledheart]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages. Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @Chilledheart)

ycmd/completers/cpp/flags.py, line 368 at r4 (raw file):

  # Libclang has already added `-xcuda` for us.
  if any( fl.lower().endswith( '.cu' ) or fl.lower().endswith( '.cuh' )

checking the lower-case version I think is good. Could we add a test that covers it?

I tried with the lower-case version but it seems the flags parsing route in compilation database couldn't truely handle with them correctly (tests added but all failed). I think it is better to handle it in another patch.

ycmd/tests/clang/diagnostics_test.py, line 399 at r1 (raw file):

Previously, Chilledheart wrote… I still see @MacOnly here?

Done.

[puremourning]

@Mergifyio rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased

[puremourning]

tests are failing

[puremourning]

Seems abandoned.