You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/07/22 22:21:33 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #8170: Remove unused URL helper functions
shinrich opened a new pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-893039513
So I started to pull the url_*_set functions into the URL methods, but those free functions are also used else where.
The url_*_get functions are not called from anywhere. So I think this PR as it stands is useful. Removing unused functions. Making the URL set methods symmetric is not beneficial since the url_*_set free functions will exist in any case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-894296454
I don't care much one way or the other. Just a tidy up PR anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich closed pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
shinrich closed pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-893039513
So I started to pull the url_*_set functions into the URL methods, but those free functions are also used else where.
The url_*_get functions are not called from anywhere. So I think this PR as it stands is useful. Removing unused functions. Making the URL set methods symmetric is not beneficial since the url_*_set free functions will exist in any case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-887569403
I think that should be a separate PR - at this point, if these are not used, it's reasonably to just KIWF.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-885743444
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-887915882
Remove the functions on this PR, and then reimplement exactly the same functions on next PR to call them from URL class? That doesn't make sense. If you're going that way, I'm going to be -1 on this PR and create "a separate PR" to avoid reimplementing the functions.
This is what you are removing
https://github.com/apache/trafficserver/blob/84cf02e6d87377d68cce8f687f6c001bc43aea47/proxy/hdrs/URL.cc#L674-L679
These are what we have in a different place
https://github.com/apache/trafficserver/blob/84cf02e6d87377d68cce8f687f6c001bc43aea47/proxy/hdrs/URL.h#L542-L548
https://github.com/apache/trafficserver/blob/84cf02e6d87377d68cce8f687f6c001bc43aea47/proxy/hdrs/URL.h#L553-L558
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-893261773
> So I started to pull the url_*_set functions into the URL methods, but those free functions are also used else where.
That is not what I'm suggesting. url_*_set functions are ok as they are.
> The url_get functions are not called from anywhere. So I think this PR as it stands is useful. Removing unused functions.
I'm suggesting to use those url_*_get functions in URL class' methods.
```
inline const char *
URL::user_get(int *length)
{
ink_assert(valid());
return url_user_get(m_url_impl, length);
}
```
No dup code, no unused code, no detail exposure, and no inconsistency. And once we do this, making all the free functions to URLImpl's methods is probably just a couple of regex operations since the first parameter is a pointer for URLImpl instance.
I think this is the right direction, because I'd have to add back the unused functions to achieve no detail exposure and no inconsistency if you remove the functions. It's not a big deal, but I'm not going to approve removing the functions for the reasons above. If you disagree find someone else to get an approval.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-893261773
> So I started to pull the url_*_set functions into the URL methods, but those free functions are also used else where.
That is not what I'm suggesting. url_*_set functions are ok as they are.
> The url_get functions are not called from anywhere. So I think this PR as it stands is useful. Removing unused functions.
I'm suggesting to use those url_*_get functions in URL class' methods.
```
inline const char *
URL::user_get(int *length)
{
ink_assert(valid());
return url_user_get(m_url_impl, length);
}
```
No dup code, no unused code, no detail exposure, and no inconsistency. And once we do this, making all the free functions to URLImpl's methods is probably just a couple of regex operations since the first parameter is a pointer for URLImpl instance.
I think this is the right direction, because I'd have to add back the unused functions to achieve no detail exposure and no inconsistency if you remove the functions. It's not a big deal, but I'm not going to approve removing the functions for the reasons above. If you disagree find someone else to get an approval.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8170: Remove unused URL helper functions
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8170:
URL: https://github.com/apache/trafficserver/pull/8170#issuecomment-886328870
We might want to call those functions from URL class' getters, instead of having the code in the getters. That's what we do for the setters, although I don't know why we need URL and URLImpl.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org