search

GNUAspell / aspell

http://aspell.net
GNU Lesser General Public License v2.1
237 stars 50 forks source link

Added support for data() call for C++11 onwards #608

Closed rishitc closed 3 years ago

rishitc commented 3 years ago

Added preprocessor check for C++ standard used for compilation. If a C++11 or after standard is detected then the data() method is used to get the pointer to the first element in the array which is used internally by the vector.

rishitc commented 3 years ago

Hi, I've just pushed another pull request and it seems to have failed the tests. I'm not sure what has caused the error this time. I added the preprocessor checks as discussed, to all the functions where I made the changes and I checked the preprocessor outputs locally as well, to confirm if the implementation is correctly selected based on the C++ standard.

Any inputs on why this would be causing code tests to fail, would be really helpful!

Thanks

rishitc commented 3 years ago

I tried the recommended changes in #609 but the tests failed again. I've quoted the description of the changes I made in that pull request, here:

I made the recommended changes but the tests still failed. I then even tried merging the &*begin() calls to data(). The vector class on compiling locally on my system, gave no errors.

Any inputs of why the tests failed would be really helpful!

I did check CircleCI, but the error messages reported are not useful. The website also says some changes need to be made to the config file to save the test results.

Thanks.

kevina commented 3 years ago

Please push your changes to this p.r.

rishitc commented 3 years ago

I have pushed my changes to this PR. As mentioned, I'll push all my further changes regarding this issue to this PR, as well.

Thanks.

kevina commented 3 years ago

Thank you for your contribution. The tests need to be fixed. I will try to get to that within the next couple of days.

Your code looks correct, but there are some minor nitpicks. To save time, once I fix the tests I will go ahead and redo your commit and push it, closing this request.

rishitc commented 3 years ago

Hi, Thank you so much for you inputs and support, while I was working on this pull request. It was really fun working on this pull request. I'll take a look at the issues list of the project and try solving some of the issues there in the coming weeks. I guess should start that, once the tests are fixed?

but there are some minor nitpicks

What nitpicks are you referring to? I would love to know, what they are, so that I can learn about them and not make those mistakes for any future PRs I submit, for other issues.

Thanks.

kevina commented 3 years ago

What nitpicks are you referring to?

You reordered the methods making it difficult to see the changes made. For such a small change it is faster for me to just fix it rather than lots of back and forth. I will be happy to answer any questions you have once I push the changes.