Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TS-4971: Change TSPluginRegistration to be const. #1110

Merged
merged 1 commit into from Oct 15, 2016
Merged

TS-4971: Change TSPluginRegistration to be const. #1110

merged 1 commit into from Oct 15, 2016

Conversation

SolidWallOfCode
Copy link
Member

No description provided.

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

jpeach
jpeach approved these changes Oct 14, 2016
View reviewed changes
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

How about removing all the const casts in plugins?

lib/ts/apidefs.h.in
char *support_email;
char const *plugin_name;
char const *vendor_name;
char const *support_email;
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything else in this file uses const char *. It would be better to keep a consistent style.

@SolidWallOfCode
Copy link
Member Author

I'll fix the const thing and do the plugin updates in a new PR. I tested the build and the plugins built fine, as expected.

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

@atsci
Copy link

atsci commented Oct 14, 2016

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

jpeach
jpeach previously requested changes Oct 15, 2016
View reviewed changes
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

I'd prefer that we focused this PR on the original problem and didn't add a lot of additional changes (at leas in the same commit). Some one these will break Solaris builds.

plugins/authproxy/authproxy.cc
{"auth-host", required_argument, 0, 'h'},
{"auth-port", required_argument, 0, 'p'},
{"auth-transform", required_argument, 0, 't'},
{"force-cacheability", no_argument, 0, 'c'},
Copy link
Contributor

Choose a reason for hiding this comment

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

This change will break Solaris.

plugins/background_fetch/background_fetch.cc
{const_cast<char *>("config"), required_argument, NULL, 'c'},
{NULL, no_argument, NULL, '\0'}};
static const struct option longopt[] = {
{"log", required_argument, NULL, 'l'}, {"config", required_argument, NULL, 'c'}, {NULL, no_argument, NULL, '\0'}};
Copy link
Contributor

Choose a reason for hiding this comment

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

Solaris again :(

plugins/esi/esi.cc
{NULL, 0, NULL, 0},
{"packed-node-support", no_argument, NULL, 'n'}, {"private-response", no_argument, NULL, 'p'},
{"disable-gzip-output", no_argument, NULL, 'z'}, {"first-byte-flush", no_argument, NULL, 'b'},
{"handler-filename", required_argument, NULL, 'f'}, {NULL, 0, NULL, 0},
Copy link
Contributor

Choose a reason for hiding this comment

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

Solaris

plugins/esi/fetcher/HttpDataFetcherImpl.cc
@@ -80,7 +80,7 @@ HttpDataFetcherImpl::addFetchRequest(const string &url, FetchedDataProcessor *ca
if (length < (int)sizeof(buff)) {
http_req = buff;
} else {
http_req = (char *)malloc(length + 1);
http_req = static_cast<char *>(malloc(length + 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced by static_cast on malloc(). It does nothing but look uglier. It would be better not to include so many different changes in this PR.

@SolidWallOfCode
Copy link
Member Author

I think we should fix up the casts, or not. I think it's worse to say we'll fix some of the pointless casts but not others. If the goal is to show correct code, they should all be fixed. I also think if we're fixing this all C style casts should be removed. Those are strongly deprecated for C++ code. The point of the distinct casts is to make it clear what is being done.

I'm fine with splitting this into a PR for each commit. That's why I left them separated.

@SolidWallOfCode SolidWallOfCode dismissed jpeach’s stale review October 15, 2016 20:50

Commits to which the review referred have been removed from the pull request.

@SolidWallOfCode SolidWallOfCode merged commit 16fe7bd into apache:master Oct 15, 2016
@atsci
Copy link

atsci commented Oct 15, 2016

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

@atsci
Copy link

atsci commented Oct 15, 2016

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

@zwoop zwoop added the TS API label Oct 18, 2016
@zwoop zwoop added this to the 7.1.0 milestone Oct 18, 2016
@SolidWallOfCode SolidWallOfCode deleted the ts-4971 branch October 21, 2018 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpeach jpeach jpeach approved these changes

Assignees

@jpeach jpeach

Labels
TS API
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@SolidWallOfCode @atsci @jpeach @zwoop