This is an archive of the discontinued LLVM Phabricator instance.

Mark getc_unlocked as unavailable by default
ClosedPublic

Authored by rprichard on Aug 4 2021, 8:13 PM.

Details

Summary

Before D45736, getc_unlocked was available by default, but turned off
for non-Cygwin/non-MinGW Windows. D45736 then added 9 more unlocked
functions, which were unavailable by default, but it also:

  • left getc_unlocked enabled by default,
  • removed the disabling line for Windows, and
  • added code to enable getc_unlocked for GNU, Android, and OSX.

For consistency, make getc_unlocked unavailable by default. Maybe this
was the intent of D45736 anyway.

Diff Detail

Event Timeline

rprichard created this revision.Aug 4 2021, 8:13 PM
rprichard requested review of this revision.Aug 4 2021, 8:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2021, 8:13 PM
rprichard updated this revision to Diff 364335.Aug 4 2021, 8:26 PM

Update test.

I noticed this inconsistency while reviewing D107509.

For the MSVC environment, Clang currently annotates getc_unlocked with nocapture, which could technically be wrong if getc_unlocked were an unrelated function. MSVC's CRT doesn't define *_unlocked functions. https://godbolt.org/z/GreGM6x8G.

(MSVC does define *_nolock functions, which look similar, but I haven't looked closely.)

This revision is now accepted and ready to land.Aug 5 2021, 11:32 AM
MaskRay accepted this revision.Aug 5 2021, 11:53 AM
MaskRay added inline comments.
llvm/test/Transforms/InferFunctionAttrs/annotate.ll
514 ↗(On Diff #364335)

What does this check?

Many times it'd be better to replace negative checks with positive checks, with a comment stating that we don't want to see certain things.

Negative checks can quite easily become stale. For example, if another function is inserted below and it has a #, the test will fail weirdly.

rprichard added inline comments.Aug 5 2021, 4:32 PM
llvm/test/Transforms/InferFunctionAttrs/annotate.ll
514 ↗(On Diff #364335)

I copied it from the similar test cases. I think it's trying to check that the function declaration doesn't have an #nnn attribute on the end. Maybe {{$}} would be a better way of doing that.

I think I'll upload that in a separate patch.

This revision was automatically updated to reflect the committed changes.