Closed Bug 1501821 Opened 11 months ago Closed 11 months ago

Update clang-plugin code to use new names getBeginLoc/getEndLoc

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: dmajor, Assigned: Waldo)

Details

Attachments

(2 files)

Some of the AST functions that we use have been renamed upstream [1] and as of r341573 the old names no longer exist [2].

We'll need to fix up the names when updating to clang trunk or 8.0.0. This may be particularly hairy now that static analysis is incorporated into our primary 
builds: is clang-plugin built by more than one clang version?

[1] http://lists.llvm.org/pipermail/cfe-dev/2018-August/058853.html
[2] https://github.com/llvm-mirror/clang/commit/cdacbfee79e73cda6a3134cafef3aa7c599da21b
Hi!  I just tried to follow my usual practice of compiling clang tip and using that (having been forced into it by the new clang 7 requirement) and ran into this.  This patch theoretically *should* allow clang-plugin to work with code before *or* after these functions were renamed.  It does seem to compile with clang tip for me, but I run into bug 1495511 so I can't fully test.
Attachment #9022323 - Flags: review?(sphink)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 9022323 [details] [diff] [review]
Implement GetBeginLoc and GetEndLoc helper functions in the clang plugin that use SFINAE to pick the function from getBeginLoc/getLocStart and getEndLoc/getLocEnd that clang actually implements

This seems like an awful lot of gymnastics for what needs to be achieved here. I would have been perfectly happy to see Andi's patch from comment 1 land. In the long term we need to use get{Begin,End}Loc exclusively. The simple #define for short-term backwards compatibility (that we can trivially rip out when ready) seemed like a more proportionate solution to me.
IMO redefining stuff is a bit more scattershot than doing the explicitly correct thing, but do whatever -- just, it'd be nice if it were done sooner rather than later so I don't have to cart around a local patch to keep building with the plugin in play.
Comment on attachment 9022323 [details] [diff] [review]
Implement GetBeginLoc and GetEndLoc helper functions in the clang plugin that use SFINAE to pick the function from getBeginLoc/getLocStart and getEndLoc/getLocEnd that clang actually implements

Review of attachment 9022323 [details] [diff] [review]:
-----------------------------------------------------------------

MozsearchIndexer.cpp needs to work without pulling in files from the parent dir, as we have a copy of that folder building standalone elsewhere that we keep in sync. So for that file at least I'd prefer an #ifdef solution.
Attachment #9022323 - Flags: review-
Jeff didnt mean to steal your patch, I’ve alresdy wrote the Fox for this when I’ve pushed to try but forgot to assign it to myself.
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/538a16d49514
Update clang-plugin in order to make it compatiable with clang 8.0.0 trunk. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/538a16d49514
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Attachment #9022323 - Flags: review?(sphink)
You need to log in before you can comment on or make changes to this bug.