Skip to content

TS-5082: define IOBufferReader::is_read_avail_more_than with TS_INLINE keyword#1252

Merged
oknet merged 1 commit intoapache:masterfrom
oknet:TS-5082
Dec 13, 2016
Merged

TS-5082: define IOBufferReader::is_read_avail_more_than with TS_INLINE keyword#1252
oknet merged 1 commit intoapache:masterfrom
oknet:TS-5082

Conversation

@oknet
Copy link
Copy Markdown
Member

@oknet oknet commented Dec 7, 2016

No description provided.

@atsci
Copy link
Copy Markdown

atsci commented Dec 7, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1254/ for details.

@atsci
Copy link
Copy Markdown

atsci commented Dec 7, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/1149/ for details.

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Dec 7, 2016

Hmmm, it seems we are incredibly inconsistent here. Why singling out this one place for this change? Do we really need TS_INLINE in general? I notice we turn off inlining via #define in some places, but then I'd argue that we should not use it at all in those place, no?

I think what I'm saying is

  1. Why not remove TS_INLINE ? I.e. just use inline.

  2. In those places where TS_INLINE is used, but #define'd to nothing, we just remove TS_INLINE.

  3. Maybe also make an effort to reduce the use of inline in general, and let the compiler deal with it.

I'm fine landing this as-is in this patch, but maybe file another Jira for a general cleanup here?

@zwoop zwoop added the Network label Dec 7, 2016
@zwoop zwoop added this to the 7.1.0 milestone Dec 7, 2016
@zwoop zwoop self-assigned this Dec 7, 2016
Copy link
Copy Markdown
Contributor

@zwoop zwoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See large comment on the bigger picture. But +1.

@oknet
Copy link
Copy Markdown
Member Author

oknet commented Dec 8, 2016

From my understand:

  • The TS_INLINE defined as inline in lib/ts/ink_apidefs.h only for libts, for libts.so these functions declared by TS_INLINE is inline function.
  • And TS_INLINE defined as empty in iocore/*/Inline.cc, for a sub module static libs (*.a) these functions declared by TS_INLINE is NOT inline function.

I don't know the reasons about the TS_INLINE macro, this case only fix the compile error.

@oknet
Copy link
Copy Markdown
Member Author

oknet commented Dec 8, 2016

A function of Event System and it is defined with TS_INLINE, e.g. bool is_read_avail_more_than()

  • It is compile into net/Inline.o and it is not inline function if it is called from Net Sub-system.
  • It is compile into eventsystem/libinkevent.a and it is inline function if it is called from Event System (internal calls).

@zwoop

@oknet
Copy link
Copy Markdown
Member Author

oknet commented Dec 8, 2016

Do I have to backport it to 6.2.x and 7.0.x ? @zwoop

@oknet oknet merged commit 662be1f into apache:master Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants