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.

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

Remove Pipeline Executor #18

Merged
merged 18 commits into from
Feb 6, 2023
Merged

Conversation

runette
Copy link
Collaborator

@runette runette commented Feb 1, 2023

Changes to Move from pdal::PipelineExector to pdal::PipelineManager since the former is deprecated.

Also - extended the ABI to include streamable execution.

@runette runette changed the title Remove pipelinexecutir Remove pipelin Executor Feb 1, 2023
@runette runette changed the title Remove pipelin Executor Remove Pipeline Executor Feb 1, 2023
@runette
Copy link
Collaborator Author

runette commented Feb 1, 2023

@hobu or @jimmysoda - since this is a relatively major change - it would be very good to get a review if you have the time

@@ -26,11 +26,13 @@
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*****************************************************************************/

#define _CRT_SECURE_NO_WARNINGS
Copy link
Member

Choose a reason for hiding this comment

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

This is usually done with compile-time definitions instead of sticking it here, but i'm not much of a windows dev

source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
@runette
Copy link
Collaborator Author

runette commented Feb 3, 2023

In the absence of any comment from @jimmysoda - I am going to go ahead and merge

source/pdal/pdalc_pipeline.cpp Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Show resolved Hide resolved
source/pdal/pdalc_pipeline.cpp Outdated Show resolved Hide resolved
@runette
Copy link
Collaborator Author

runette commented Feb 4, 2023

@abellgithub - could you possibly "spot me" as they would say in climbing (i.e. check my reasoning) since I am to a large extent reverse engineering some stuff here :

About null terminating strings etc as per review comment5s:

  1. looking at the C# bindings it is clear that they, at least, DO need guaranteed null terminated strings,
  2. c_str() will guarantee a null terminated string except, of course, that strncpy does not then preserve that if size of string > size of buffer,
  3. I am guessing that this is what was happening with the "interesting" initialisation of the buffer - to guarantee the null termination.
  4. However - this was inconsistently applied and anyway is not very "readable".
  5. So I have changed all instances to :
    a - get the string
    b - resize the string to max length (size -1) (to allow for the null character),
    c - convert using c_str() which should guarantee a valid null terminated string of max length of size.

Is my logic valid?

Thanks

@runette runette merged commit 71254eb into PDAL:master Feb 6, 2023
@runette runette deleted the remove-pipelinexecutir branch February 6, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants