apache / trafficserver

Apache Traffic Serverâ„¢ is a fast, scalable and extensible HTTP/1.1 and HTTP/2 compliant caching proxy server.
https://trafficserver.apache.org/
Apache License 2.0
1.74k stars 782 forks source link

Deprecate name_get() and value_get() function returns `const char *` #10153

Open masaori335 opened 11 months ago

masaori335 commented 11 months ago

Proposal

Deprecate below two functions of the MIMEField

const char *name_get(int *length) const;
const char *value_get(int *length) const;

Background

MIMEField has two value_get functions return std::string_view and const char *.

https://github.com/apache/trafficserver/blob/f9aa1e0adb7d23cb9d9dd4fb421b80a24654ae85/proxy/hdrs/MIME.h#L162-L164

Discussion on #10144, std::string_view value_get() const; is preferred for error handling and I completely agree with using std::string_view.

We can say the same things for name_get too. https://github.com/apache/trafficserver/blob/f9aa1e0adb7d23cb9d9dd4fb421b80a24654ae85/proxy/hdrs/MIME.h#L144-L146

Concerns

These functions are used widely. The change will be big.

masaori335 commented 11 months ago

Note from weekly bug scrub:


@bryancall raised a concern about performance impact. Because he saw a performance bottleneck with std::string_view version before.

However, the current implementation of the const char * version is calling the std::string_view version internally, as @SolidWallOfCode pointed out. https://github.com/apache/trafficserver/blob/f9e931441e73eb1927ab3d7b60a5c77f762cdbb8/proxy/hdrs/MIME.h#L991-L997

SunMoon97 commented 10 months ago

@masaori335 Can you please assign this to me,I would like to work on this.

ShaiviAgarwal2 commented 7 months ago

@masaori335 is the above issue resolved? If not, I would like to contribute to it.

masaori335 commented 7 months ago

This is not resolved yet. Thanks for showing interests!

@SunMoon97 I'm really sorry, I just noticed your comment. Still want to work on this? If so, please go a head.

@ShaiviAgarwal2 if @SunMoon97 doesn't want to work on this or no reply for a while, please work on this. I'd also say we have more Good First Issues :) Good First Issue

ShaiviAgarwal2 commented 7 months ago

Okk

Manohar-Penta commented 7 months ago

Hello. can someone pls explain where can i find the definition of functions std::string_view name_get() const; and std::string_view value_get() const; in the code. I couldn't find the code for them.

ShaiviAgarwal2 commented 7 months ago

Hello. can someone pls explain where can i find the definition of functions std::string_view name_get() const; and std::string_view value_get() const; in the code. I couldn't find the code for them.

The functions std::string_view name_get() const;and std::string_view value_get() const; are member functions of the MIMEField class. The definitions of these functions would be in the corresponding source file (MIME.cc), which is typically located in the same directory as the header file (MIME.h). According to me, name_get() returns the name of the field, and value_get()returns the value of the field. Both functions include parameters to return the length of the name or value as an integer. These functions are defined in the header file for the MIMEField class

Deprecating the functions means marking them as outdated or no longer recommended for use. It is a way to inform users that these functions may be removed in future versions and should be replaced with the preferred alternatives.

ShaiviAgarwal2 commented 7 months ago

@Manohar-Penta I hope I was able to solve your doubt!! If not, please acknowledge the message

Manohar-Penta commented 7 months ago

Thanks Shaivi. I was able to find the definitions of the functions. and Are you still working on the issue?? If not can I take over?

dmefs commented 1 month ago

Hi, can I tackle this issue?

dmefs commented 1 month ago

Should I change char * version to std::string_view in all places or adding a warning is enough?