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

[csv-tree-export.cpp] convert to cpp, use fstream #1598

Merged
merged 3 commits into from Apr 5, 2023

Conversation

christopherlam
Copy link
Contributor

instead of multiple char* malloc and freeing, write to fstream directly

@christopherlam christopherlam force-pushed the csv-tree-export-cpp branch 6 times, most recently from 25cbdf3 to a143425 Compare April 2, 2023 02:27
@christopherlam christopherlam marked this pull request as ready for review April 2, 2023 08:25
@christopherlam christopherlam force-pushed the csv-tree-export-cpp branch 2 times, most recently from bb7f648 to afd1a0f Compare April 3, 2023 03:07
@christopherlam
Copy link
Contributor Author

Here's a cpp conundrum. This branch will produce output containing garbage eg.

Date,Transaction ID,Number,Description,Notes,Commodity/Currency,Void Reason,Action,Memo,Full Account Name,Account Name,Amount With Sym,Amount Num.,Value With Sym,Value Num.,Reconcile,Reconcile Date,Rate/Price
04/02/23,~Ãâ_����15554f57a8cd3bc0627a5b02,,ZLR*Fresh Finesse,OFX ext. info: |Trans type:Other,CURRENCY::AUD,,,,Imbalance-AUD,Imbalance-AUD,$5.00,5.00,$5.00,5.00,n,,"À""aÈÿ�"
04/02/23,~Ëâ_����15554f57a8cd3bc0627a5b02,,ZLR*Fresh Finesse,OFX ext. info: |Trans type:Other,CURRENCY::AUD,,,ZLR*Fresh Finesse - SUBIACO AUS,N�PsûU��y:Westpac Creditcard,R�µtûU��Creditcard,-$5.00,-5.00,-$5.00,-5.00,c,,"À""aÈÿ�"
11/02/23,~Ãâ_����1447453382319c3bbbad3528,,ZLR*Fresh Finesse,OFX ext. info: |Trans type:Other,CURRENCY::AUD,,,,Imbalance-AUD,Imbalance-AUD,$5.00,5.00,$5.00,5.00,n,,"À""aÈÿ�"
11/02/23,~Ëâ_����1447453382319c3bbbad3528,,ZLR*Fresh Finesse,OFX ext. info: |Trans type:Other,CURRENCY::AUD,,,ZLR*Fresh Finesse - SUBIACO AUS,þ�AsûU��y:Westpac Creditcard,�åÚsûU��Creditcard,-$5.00,-5.00,-$5.00,-5.00,c,,"À""aÈÿ�"

The offending fields are generated via e.g. get_guid or get_category or get_price, and these functions will output an std::string in make_complex_trans_line. These std::string or std::string_view are collected in StringVec which is a std::vector<std::string_view>. This stringvec is then pasted into the fstream.

It's my understanding that casting std::string to std::string_view is inexpensive, and I can't figure out why they are mangled between e.g. get_guid which outputs a clean guid string, and make_complex_trans_line which outputs a vector containing a mangled string.

@jralls
Copy link
Member

jralls commented Apr 3, 2023

It's my understanding that casting std::string to std::string_view is inexpensive, and I can't figure out why they are mangled between e.g. get_guid which outputs a clean guid string, and make_complex_trans_line which outputs a vector containing a mangled string.

A std::string_view doesn't own its memory so it's like a reference or a pointer: The thing that it refers to has to be valid throughout the string_view's lifetime, but like pointers and unlike references the pointed-to objects don't get their lifetimes extended, so your StringVec (and you should call a std::vectorstd::string_view a StringVVec to make clear that it doesn't own its objects) has what amount to a bunch of pointers to old stack frames. Some of them happen to be OK, but the longer you hang on to them the riskier they are.

Make StringVec a std::vector<std::string> and copy elision will ensure that the strings are constructed in-place in the vector and everything will be properly cleaned up when the vector goes out of scope. If you need to pass one of the StringVec's elements to some function by name you could use a string_view, but in most cases a const std::string& is cheaper and easier to deal with.

String_views are great if you want a cheap way to wrap a const char* so that you can use std::string semantics on it, or if you want to get a bunch of substrings from a const std::string without copying the characters into a new string. Just be mindful of the lifetime of the original!

@christopherlam
Copy link
Contributor Author

christopherlam commented Apr 3, 2023

It's my understanding that casting std::string to std::string_view is inexpensive, and I can't figure out why they are mangled between e.g. get_guid which outputs a clean guid string, and make_complex_trans_line which outputs a vector containing a mangled string.

A std::string_view doesn't own its memory so it's like a reference or a pointer: The thing that it refers to has to be valid throughout the string_view's lifetime, but like pointers and unlike references the pointed-to objects don't get their lifetimes extended, so your StringVec (and you should call a std::vectorstd::string_view a StringVVec to make clear that it doesn't own its objects) has what amount to a bunch of pointers to old stack frames. Some of them happen to be OK, but the longer you hang on to them the riskier they are.

Make StringVec a std::vector<std::string> and copy elision will ensure that the strings are constructed in-place in the vector and everything will be properly cleaned up when the vector goes out of scope. If you need to pass one of the StringVec's elements to some function by name you could use a string_view, but in most cases a const std::string& is cheaper and easier to deal with.

String_views are great if you want a cheap way to wrap a const char* so that you can use std::string semantics on it, or if you want to get a bunch of substrings from a const std::string without copying the characters into a new string. Just be mindful of the lifetime of the original!

Ok best use vector<string> then. I did try leaving some e.g. xaccTransGetDescription as std::string_view hoping they'd get copied into std::string but the StringVec constructor got confused and refused to compile. Best make them all output std::strings. Now it works well :)

@christopherlam christopherlam force-pushed the csv-tree-export-cpp branch 3 times, most recently from db4e213 to ed34016 Compare April 3, 2023 23:23
@code-gnucash-org code-gnucash-org merged commit e5349a8 into Gnucash:stable Apr 5, 2023
3 checks passed
@christopherlam christopherlam deleted the csv-tree-export-cpp branch April 5, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants