kdev-clang : somewhat more complete ObjC(++) support
Needs RevisionPublic

Authored by rjvbb on Sep 15 2018, 12:07 PM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Summary

This improves ObjC support:

  • by recognising additional mimetypes (also in the source formatter)
  • by changing the logic in argsForSession() such that the ObjC and ObjC++ mode arguments are appended to the other C/C++ arguments rather than replacing them.

Those are probably the main changes that give a significant improvement. In addition, I added code for handling two more CXCursor_* types (a bit via educated guessing). Handling CXCursor_BlockExpr as a lambda expr. seems rather evident, it is much less clear how to handle CXCursor_ObjCMessageExpr as I couldn't find a generic C/C++ expression handler (nor a corresponding CXCursor type, btw).

Test Plan

Example project: github:mulle-nat/mulle-xcode-to-cmake (requires gnustep-base to be installed).

Parsing of the project's ObjC file seems much more complete with my patch (once you get KDevelop to recognise .m files as ObjC by default). There's still plenty room for improvement though:

  • the outline navigator drop-down list doesn't include any ObjC methods
  • instance and class functions appear as "invalid type" in context browser popups, both when hovering over their definition or when they're used as a selector in a message sent to an instance variable (or class object).
  • Class categories (e.g. @implementation NSString (ExternalName)) are promoted to a class of their own, so methods in that category are listed as, for example, belonging to the class ExternalName instead of the NSString class to which they actually belong.
  • sticking with this example from the project above: the outline widget shows current ObjC methods inappropriately in C++ syntax, e.g. ExternalName::externalNameForInternalName:separatorString:useAllCaps:
  • selectors are sometimes mistaken for functions of the same name
  • in an expression like [NSMutableString string], NSMutableString shows up as a function (in this case, the string method) instead of as a class object (or instead of an instance variable in expressions like [s length]).

Fixing those on my own is out of my league, but I'm willing to lend a hand.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7510
Build 7528: arc lint + arc unit
rjvbb requested review of this revision.Sep 15 2018, 12:07 PM
rjvbb created this revision.
kossebau added inline comments.
plugins/astyle/astyle_plugin.cpp
104 ↗(On Diff #41693)

Please split that out into a separate commit/review, unrelated to kdev-clang & can be possibly quickly handed (as fix for 5.3 even?).

rjvbb updated this revision to Diff 41697.Sep 15 2018, 12:21 PM

astye change split off as D15532

rjvbb set the repository for this revision to R32 KDevelop.Sep 15 2018, 12:22 PM
rjvbb updated this revision to Diff 42007.Sep 20 2018, 8:51 PM

A small additional change to the plugin json, and a request for a bit of help.

In most cases the parser will only show a supposed forward declaration for ObjC classes, at the proper line nr. for (usually) the @interface definition. That's not correct, and I think it's because of the way CXCursorKind tokens are handled in cursorkindtraits.h . I tried to look into that but haven't been able to figure out how to make relevant changes to that file and not get a ton of compiler errors (which I also don't understand).

http://andrewd.ces.clemson.edu/courses/cpsc102/notes/ObjC.pdf

rjvbb set the repository for this revision to R32 KDevelop.Sep 20 2018, 8:52 PM

You should probably also add a few lines to the custom-definesandincludes plugin.

plugins/clang/duchain/parsesession.cpp
114–115

This seems to be relevant for unit tests, why do you do the distinction here? I think it should be done down where my other comment is.

134–146

Maybe the time has come to make this a switch statement.

You should probably also add a few lines to the custom-definesandincludes plugin.

In a different review I presume?

plugins/clang/duchain/parsesession.cpp
114–115

I didn't want to presume that parserSettings.parserOptions can never be empty for other reasons, and didn't want to change the logic completely where ObjC presence would override parserOptions.isEmpty() (which causes a return).

134–146

Maybe as a separate, later change, or is there already a state or language variable that is suitable an argument for a switch statement?

rjvbb updated this revision to Diff 42045.Sep 21 2018, 10:33 AM

Updated as suggested. No switch yet but a layout that looks more like one.

rjvbb set the repository for this revision to R32 KDevelop.Sep 21 2018, 10:33 AM

You should probably also add a few lines to the custom-definesandincludes plugin.

In a different review I presume?

Of course, keep changes small.

plugins/clang/duchain/parsesession.cpp
134–146

Thanks, that's exactly what I meant. It doesn't literally have to be a switch statement. (Although it would be a nice refactoring to add an enumeration later.)

rjvbb marked 4 inline comments as done.Sep 22 2018, 8:20 AM

You should probably also add a few lines to the custom-definesandincludes plugin.

Of course, keep changes small.

I've made them locally, but am not certain I'm seeing any effect (at least not after I fixed the bug that caused the illegal "-xobjective-c -std=c++11" combo). Should I?

Can someone please look into my current blocker, the mis-identification of @interface <classname> class definitions as forward declarations? I'm really flabbergasted by the fact I can't change the call to isClass() to a call to a new function isClassDecl() in isForwardDeclaration()...

As mentioned on https://phabricator.kde.org/D15532#330528, text/x-objchdr & text/x-objc++src are MIME types no-where defined from what I saw. Which makes things irritating for future code readers, as well as resulting in annpying warnings in the runtime log.
Please keep those MIME types out of this patch, until that has been resolved.

rjvbb added a comment.Sep 23 2018, 3:59 PM

Doesn't KDevelop already add (install) a few mimetypes of its own? I think I mentioned before that I do have a .xml file for SMI, adding the x-objc++src definition and I presume that's why I am not seeing mimetype warnings about ObC (I'm seeing others though.)

Installing our own is the best way to be certain the definition is there - cf. the discussion around the astyle library and e.g. Debian Stable.

As to x-objchdr, I'm not certain. In fact I even wonder how a difference can be made between x-chdr and x-c++hdr ... but if that's indeed possible it should be trivial to add something for x-objchdr too.

Doesn't KDevelop already add (install) a few mimetypes of its own? I think I mentioned before that I do have a .xml file for SMI, adding the x-objc++src definition and I presume that's why I am not seeing mimetype warnings about ObC (I'm seeing others though.)

Installing our own is the best way to be certain the definition is there - cf. the discussion around the astyle library and e.g. Debian Stable.

Sure, I am mainly interested here that we use MIME type ids which also accepted elsewhere, and not invent our own here and ignore the rest of the world :)
Once e.g. shared-mime-info has some version accepted, we can then provide fall-back variants in the kdevelop s-m-i files installed.

As to x-objchdr, I'm not certain. In fact I even wonder how a difference can be made between x-chdr and x-c++hdr ... but if that's indeed possible it should be trivial to add something for x-objchdr too.

At least with s-m-i the header files are "recognized" using the file suffix, which of course makes C++ headers using ".h" being mis-identified (which has become one reason for some to not use .h with C++ headers). Convenience link to the rules: https://cgit.freedesktop.org/xdg/shared-mime-info/tree/freedesktop.org.xml.in
So if there is a special file suffix for x-objchdr (which IMHO there should be just for sanity), there is at least one chance.

rjvbb added a comment.Sep 24 2018, 7:40 AM
Sure, I am mainly interested here that we use MIME type ids which also accepted elsewhere, and not invent our own here and ignore the rest of the world :)
Once e.g. shared-mime-info has some version accepted, we can then provide fall-back variants in the kdevelop s-m-i files installed.

I don't understand, that sounds like a contradiction. As soon as s-m-i has some version accepted we could stop providing our own code (when that version has become common in distroland), is that what you wanted to say?
And what could be different in "their" official version - isn't an s-m-i entry just a slightly glorified extension mapping? (Which if "they" get their way I would implement from A-to-Z going through alpha to omega.)

I'll try to figure out today if you're correct in thinking that Qt on Mac uses a different source for mimetypes and if I can learn anything from that. If x-obj++src is defined there we could use it there at least, no?

At least with s-m-i the header files are "recognized" using the file suffix, which of course makes C++ headers using ".h" being mis-identified

I've always wondered about that, but somehow it doesn't seem to be an issue in KDevelop.

So if there is a special file suffix for x-objchdr (which IMHO there should be just for sanity), there is at least one chance.

I have never seen anything of the kind, which is a bit strange given that ObjC does introduce its own #import keyword that one is supposed to use instead of #include. Even Apple's own headers all use .h, presumably because quite a few of them can be imported in regular C(++) code too.
At least they *have* an extension, contrary to "native" C++ headers ;)

Sure, I am mainly interested here that we use MIME type ids which also accepted elsewhere, and not invent our own here and ignore the rest of the world :)
Once e.g. shared-mime-info has some version accepted, we can then provide fall-back variants in the kdevelop s-m-i files installed.

I don't understand, that sounds like a contradiction. As soon as s-m-i has some version accepted we could stop providing our own code (when that version has become common in distroland), is that what you wanted to say?

Yes. I meant, once we have something outside of kdevelop accepted=recognized as MIME type id, we then know what to use ourselves as well.
So let's first please do some effort to have a MIME type id agreed on by others (ideally this would be something done in the Objective-C++ community where you might have your contacts to poke), then we can rely on the result here (in our intermediate s-m-i data installed).

And what could be different in "their" official version - isn't an s-m-i entry just a slightly glorified extension mapping?

The main feature of s-m-i is to have some database with MIME type definitions everyone uses, so things are compatible, especially when it comes to the MIME type name/id.
And yes, it would be great to have more magic inside the database, but if no-one works on that, it's often only extension mapping.
Still s-m-i is better than nothing and better than lots of incompatible-id bubbles.

At least with s-m-i the header files are "recognized" using the file suffix, which of course makes C++ headers using ".h" being mis-identified

I've always wondered about that, but somehow it doesn't seem to be an issue in KDevelop.

At least the source-formatter is failing on C++ headers named .h and treating them as C, as it relies on info from QMime* which uses only shared-mime-info db (where there is no proper magic around to tell if it's not rather C++).

rjvbb added a comment.Sep 25 2018, 8:37 PM
So let's first please do some effort to have a MIME type id agreed on by others (ideally this would be something done in the Objective-C++ community where you might have your contacts to poke)

Given that x-objcsrc already exists (in s-m-i) I think there's little chance that something other than x-objc++src would be chosen. But you may have seen that I filed a ticket on Qt's bugtracker. It turns out they bundle a copy of the freedesktop.org.xml db for use on platforms that don't have s-m-i installed (where Qt can find it). That includes the Mac platform. This copy also lacks a definition for ObjC++, yet the Qt sources are the largest corpus of files written in that language that I am aware of, so it would make sense for them to include a mimetype for it in their own mimetypes db.

Also, the patch I proposed upstream (to s-m-i) has been in use in MacPorts' port:shared-mime-info; the type id has never been a topic of debate there. I also notice that Qt Creator uses x-objc++src too.

rjvbb updated this revision to Diff 42357.Sep 26 2018, 10:33 AM

Removed the x-objchdr mimetype (to avoid additional confusion what a .h file could be) and added my current x-objc+src definition to kdevclang.xml .

rjvbb set the repository for this revision to R32 KDevelop.Sep 26 2018, 10:33 AM
mwolff requested changes to this revision.Jan 24 2019, 3:55 PM
mwolff added a subscriber: mwolff.

please add tests for the features you add - it's probably enough to add one or two files to tests/files/ and use the JSON comments to verify everything is parsed properly?

plugins/clang/duchain/builder.cpp
1207

space before ?

plugins/clang/duchain/cursorkindtraits.h
62

so can this be removed? if not, what compile errors do you get?

plugins/clang/duchain/parsesession.cpp
92–93

const auto mimeType = ...
and then move down to where this is actually used

111

by moving down the check you break this path, but apparently that will only show up in unit tests

134–145

... but do we really want to move these checks down here? is it OK to pass the -nostdinc command e.g. for objc?

plugins/clang/kdevclang.xml
67

remove all the translations, the KDE translation team will handle this

This revision now requires changes to proceed.Jan 24 2019, 3:55 PM
rjvbb marked 2 inline comments as done.Jan 25 2019, 12:41 PM
rjvbb added inline comments.
plugins/clang/duchain/cursorkindtraits.h
62

Sadly, no, it can't go and I still don't understand a thing of the why and how. 35y of programming tell me I should be able to remove the || CK==CXCursor_ObjC* lines from the function above the commentb but when I do event that I get this:

I get the same errors if instead I call isClassNoObjc() instead of isClass() in isKDevForwardDeclaration(), also when actually building the files.

This goes way beyond my template-fu, hence the comment.

plugins/clang/duchain/parsesession.cpp
111

Sorry, comments and code seem to have gone out of sync so this is a bit cryptic. What path do I break? I can see how unittests that didn't set parserOptions will not get relevant options for ObjC so I've tried to address that.
(The out-commented lines are just there to make it easier to revert this change if needed, they'll go otherwise).

134–145

It is; ObjC is a superset of C and ObjC++ is a superset of C++ so to the best of my knownledge compiler options for the parent language are OK.

I suppose the alternative is to move the checks upwards so that the options are prepended to the session arguments list, which will be a bit easier with the newly reorganised code. I have slight preference for appending because that ensures the parser is always switched to ObjC mode when the mimetype says it should.

plugins/clang/kdevclang.xml
67

I presume you mean "all translations you added"? :)

rjvbb marked an inline comment as done.Jan 25 2019, 12:50 PM

Where or how can I find what to put in the JSON comments?

rjvbb updated this revision to Diff 50253.Jan 25 2019, 1:57 PM

Updated as discussed.

rjvbb set the repository for this revision to R32 KDevelop.Jan 25 2019, 1:58 PM
mwolff requested changes to this revision.Jan 27 2019, 7:52 PM

please paste compiler errors instead of showing screenshots

plugins/clang/duchain/parsesession.cpp
113

yep remove this, I hope it's all fine (did you check that there are no new tests failures?)

This revision now requires changes to proceed.Jan 27 2019, 7:52 PM
rjvbb marked 2 inline comments as done.Feb 4 2019, 10:06 AM
rjvbb added inline comments.
plugins/clang/duchain/parsesession.cpp
113

I don't see any, no. Are there expected failures that are not labelled as such, or any tests I should check in particular?

I only get a cuda-related failure in the test_files test, and that's probably unrelated to my changes and due to

error: cannot find CUDA installation.  Provide its path via --cuda-path, or pass -nocudainc to build without CUDA includes.
error: cannot find libdevice for sm_20. Provide path to different CUDA installation via --cuda-path, or pass -nocudalib to build without linking with libdevice.
error: cannot find CUDA installation.  Provide its path via --cuda-path, or pass -nocudainc to build without CUDA includes.
rjvbb updated this revision to Diff 50825.Feb 4 2019, 10:07 AM
rjvbb marked an inline comment as done.

Comment removed.

rjvbb set the repository for this revision to R32 KDevelop.Feb 4 2019, 10:07 AM
mwolff requested changes to this revision.Feb 5 2019, 7:32 AM
mwolff added inline comments.
plugins/clang/duchain/cursorkindtraits.h
58

I'm still waiting for a textual form of the compiler errors you are seeing

This revision now requires changes to proceed.Feb 5 2019, 7:32 AM