This is an archive of the discontinued LLVM Phabricator instance.

[LLParser] Remove outdated deplibs
ClosedPublic

Authored by TH3CHARLie on May 19 2021, 5:40 AM.

Details

Summary

The comment mentions deplibs should be removed in 4.0. Removing it in this patch.

Diff Detail

Event Timeline

TH3CHARLie created this revision.May 19 2021, 5:40 AM
TH3CHARLie requested review of this revision.May 19 2021, 5:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 5:40 AM
TH3CHARLie edited the summary of this revision. (Show Details)

fix compliation errors

Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 6:37 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
compnerd requested changes to this revision.May 19 2021, 9:13 AM
compnerd added a subscriber: compnerd.

While I can appreciate the removal of deprecated functionality, I think that some sort of mention of this on llvm-dev at least 2 weeks prior to the removal is warranted. It impacts any downstream projects which may still be relying on a feature.

libcxxabi/test/test_demangle.pass.cpp
12418 ↗(On Diff #346434)

This should not be removed, its a name undecoration test case.

This revision now requires changes to proceed.May 19 2021, 9:13 AM

Hi! Thanks for reviewing this revision. It's my first attempt to refactor the lib codebase. You are right and I wasn't aware the potential impact. I'll file email to llvm-dev and see if we can proceed this refactoring two weeks later.

Revert removed test

ldionne removed a reviewer: Restricted Project.May 28 2021, 8:45 AM

Hi @compnerd , I've posted a thread (https://lists.llvm.org/pipermail/llvm-dev/2021-May/150690.html) at llvm-dev two weeks ago and received some positive feedbacks, could you please take another look and see if we can move forward?

Hi @MaskRay could you please take a look? thanks!

MaskRay edited reviewers, added: dexonsmith; removed: MaskRay.Jun 3 2021, 8:38 PM

Hi @lattner I wonder if you could review this patch given you support this on llvm-dev email list. Thanks in advance!

compnerd accepted this revision.Jun 12 2021, 9:13 AM

Given the thread on the mailing list, I think that this is fine as long as there is an accompanying test to validate the bitcode deserialization continues to work. Id wait a few days more to see if @dexonsmith has an opinion.

This revision is now accepted and ready to land.Jun 12 2021, 9:13 AM
dexonsmith accepted this revision.Jun 13 2021, 12:55 PM

Given the thread on the mailing list, I think that this is fine as long as there is an accompanying test to validate the bitcode deserialization continues to work. Id wait a few days more to see if @dexonsmith has an opinion.

LGTM too (sorry for any delay; on vacation).

Note two historical quirks about this "Remove in 4.0" comment, which has experienced some bitrot.

  1. In the past, the community assumed that 4.0 would break compatibility with 3.x (like 3.0 did with 2.x). We since dropped that (and went from 3.9 to 4.0 without breaking compatibility), but I guess some of the boilerplate comments are still around. I think such comments in the bitcode parser were already removed since we don't anticipate breaking compatibility "ever".
  2. In the past, LLVM parsed (and auto-upgraded) old textual IR. At some point we dropped that requirement. We still parse some non-canonical forms of textual IR as a convenience (mainly to avoid having to update old testcases), but we're allowed to break compatibility when it "makes sense", unrelated to specific version numbers.
lattner accepted this revision.Jun 13 2021, 2:16 PM

Thanks!

Thank everyone for the reviews and further clarification, I believe we reach an agreement this is ready to land. committing it now.

This revision was automatically updated to reflect the committed changes.