You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "ywkaras (via GitHub)" <gi...@apache.org> on 2023/08/16 23:21:44 UTC
[GitHub] [trafficserver] ywkaras opened a new pull request, #10231: Move Cleanup.h from include/tscpp/api to include/ts.
ywkaras opened a new pull request, #10231:
URL: https://github.com/apache/trafficserver/pull/10231
Change the namespace of its declarations from atscppapi to tsapi::c_support. This is in support of removing the main, high-overhead parts of the ATS C++ API.
I used `git mv` to move the file, frustrating that github shows it as a deletion and a new file.
--
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] ywkaras commented on pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1682473331
> Should this go in the new `include/api` directory?
I haven't heard about that directory, maybe I wasn't in the right meeting (or I slipped into a meeting coma).
--
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] ywkaras commented on pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1688651039
> You proposed removing C++ API and saving Cleanup.h, but promoting Cleanup.h to TS API is not part of the proposal. You might have meant it, but I didn't think that is included. And the thread is about "Removing example/plugins/cpp-api" and it's not even started by you. I don't think it's a valid proposal.
https://lists.apache.org/list?dev@trafficserver.apache.org:2023-8
--
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] ywkaras commented on a diff in pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#discussion_r1301597238
##########
include/ts/Cleanup.h:
##########
@@ -0,0 +1,201 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+/**
+ * @file Cleanup.h
+ * @brief Easy-to-use utilities to avoid resource leaks or double-releases of resources.
+ */
+
+#pragma once
+
+#include <type_traits>
+#include <memory>
+
+#include <ts/ts.h>
+
+namespace tsapi
+{
+// This sub-namespace is for declarations specifically designed to work with declarations in tsapi::c.
Review Comment:
In ts.h.
--
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 #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1682750352
> It will be part of the TS API.
Then it's an addition in terms of TS API, and a proposal need to be sent on the dev list.
--
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 a diff in pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#discussion_r1301934361
##########
include/ts/Cleanup.h:
##########
@@ -0,0 +1,201 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+/**
+ * @file Cleanup.h
+ * @brief Easy-to-use utilities to avoid resource leaks or double-releases of resources.
+ */
+
+#pragma once
+
+#include <type_traits>
+#include <memory>
+
+#include <ts/ts.h>
+
+namespace tsapi
+{
+// This sub-namespace is for declarations specifically designed to work with declarations in tsapi::c.
Review Comment:
Ok, so it was added recently. I didn't know such namespace was added. I don't think I saw the proposal on the list.
--
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] ywkaras commented on pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1682241201
> Does this mean everything in Cleanup.h is going to be part of TS API and require API proposal and deprecation period when we change it? Or is it still just an utility that does not guarantee any compatibility between ATS versions?
Yes I think so.
--
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 #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1682504529
> > Does this mean everything in Cleanup.h is going to be part of TS API and require API proposal and deprecation period when we change it? Or is it still just a utility that does not guarantee any compatibility between ATS versions?
> Yes I think so.
Which one?
--
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 #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1683129219
You mean the one you sent last year?
--
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 a diff in pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#discussion_r1297858415
##########
include/ts/Cleanup.h:
##########
@@ -0,0 +1,201 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+/**
+ * @file Cleanup.h
+ * @brief Easy-to-use utilities to avoid resource leaks or double-releases of resources.
+ */
+
+#pragma once
+
+#include <type_traits>
+#include <memory>
+
+#include <ts/ts.h>
+
+namespace tsapi
+{
+// This sub-namespace is for declarations specifically designed to work with declarations in tsapi::c.
Review Comment:
Where's `tsapi::c`?
--
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 #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1681396551
Does this mean everything in Cleanup.h is going to be part of TS API and require API proposal and deprecation period when we change it? Or is it still just an utility that does not guarantee any compatibility between ATS versions?
--
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] ywkaras commented on pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1682734055
> > > Does this mean everything in Cleanup.h is going to be part of TS API and require API proposal and deprecation period when we change it? Or is it still just a utility that does not guarantee any compatibility between ATS versions?
>
> > Yes I think so.
>
> Which one?
It will be part of the TS API.
--
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] cmcfarlen commented on pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1682430746
Should this go in the new `include/api` directory?
--
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] ywkaras commented on pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1683107100
I have brought it up on the mailing list.
On Thu, Aug 17, 2023 at 2:15 PM Masakazu Kitajo ***@***.***>
wrote:
> It will be part of the TS API.
>
> Then it's an addition in terms of TS API, and a proposal need to be sent
> on the dev list.
>
> —
> Reply to this email directly, view it on GitHub
> <https://urldefense.com/v3/__https://github.com/apache/trafficserver/pull/10231*issuecomment-1682750352__;Iw!!Op6eflyXZCqGR5I!FXYXMjLIzshh0-W1SWqHatXg4jEodEH_NFQuUYJcmh3ppXip-xdvoLJtV5TWXCcza_p5n-UYN1StaJK2NWwilU5I$>,
> or unsubscribe
> <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AG6ILQ64FHNLZZMWRYWYFJ3XVZNVXANCNFSM6AAAAAA3TG5KDM__;!!Op6eflyXZCqGR5I!FXYXMjLIzshh0-W1SWqHatXg4jEodEH_NFQuUYJcmh3ppXip-xdvoLJtV5TWXCcza_p5n-UYN1StaJK2NbTapmBk$>
> .
> You are receiving this because you were assigned.Message ID:
> ***@***.***>
>
--
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
Re: [PR] Move Cleanup.h from include/tscpp/api to include/ts. [trafficserver]
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras closed pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
URL: https://github.com/apache/trafficserver/pull/10231
--
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] ywkaras commented on pull request #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1688139503
> You mean the one you sent last year?
https://lists.apache.org/list?dev@trafficserver.apache.org:2023-8
--
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 #10231: Move Cleanup.h from include/tscpp/api to include/ts.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #10231:
URL: https://github.com/apache/trafficserver/pull/10231#issuecomment-1688569141
You proposed removing C++ API and saving Cleanup.h, but promoting Cleanup.h to TS API is not. You might have meant it, but I didn't think that is included. And the thread is about "Removing example/plugins/cpp-api" and it's not even started by you. I don't think it's a valid proposal.
--
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