1318955 - Upgrade to Hunspell 1.5.4

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Changes since v1.4.1:

2016-11-18 Dimitrij Mijoski <dmjpp at hotm>:
 * Version 1.5
 * Lot of stability fixes
 * Fixed compilation errors on various systems (Windows, FreeBSD)
 * Small performance improvement compared to 1.4.0
 * API is same as 1.4.

2016-04-29 Caolán McNamara <caolanm at LibO>:
 * deprecate old api and add new one
   old one remains implemented in terms of new one
   and will eventually be removed
 * shrink exposed api down to just hunspell.hxx
 * next major release is likely to require C++11

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Update Hunspell to version 1.5.0

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b299b805f3b367a6c0dc279b670c250060a2dc9c

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8812590 [details] [diff] [review]
Update Hunspell to version 1.5.0

I've updated the version in README.mozilla to 1.5.0 locally, sorry for forgetting to do that before submitting.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8812590 [details] [diff] [review]
Update Hunspell to version 1.5.0

>diff --git a/extensions/spellcheck/hunspell/glue/mozHunspell.cpp b/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>--- a/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>+++ b/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>@@ -195,17 +195,17 @@ NS_IMETHODIMP mozHunspell::SetDictionary
>   mDictionary = aDictionary;
>   mAffixFileName = affFileName;
> 
>   mHunspell = new Hunspell(affFileName.get(),
>                          dictFileName.get());
>   if (!mHunspell)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>-  nsDependentCString label(mHunspell->get_dic_encoding());
>+  nsCString label(mHunspell->get_dict_encoding().c_str());

Why don't you use nsAutoCString?

>-nsresult mozHunspell::ConvertCharset(const char16_t* aStr, char ** aDst)
>+nsresult mozHunspell::ConvertCharset(const char16_t* aStr, std::string* aDst)

nit: I'd like you to break the line after |nresult|.


I assume that the changes under extensions/spellcheck/hunspell/src comes from upstream. So, I don't check them, if I should do that, let me know.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> I assume that the changes under extensions/spellcheck/hunspell/src comes
> from upstream. So, I don't check them, if I should do that, let me know.

Correct. You're certainly welcome to if you want, though! :)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Tyson, have you been fuzzing Hunspell lately?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Attachment: Update Hunspell to version 1.5.0

This addresses your review comments. Still green on Try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7248e0dcc8157529e36e66a4a77816451efa9c6&group_state=expanded

While changing the nsCString to nsAutoCString, I noticed a couple instances in mozHunspell.cpp as well. Is there any point in changing those while I'm at it? Looks OK on Try as well.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a79569a89fdb7f7257dd34382fed2bc43bf2ff98

Thanks for the quick review!

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

The reason why I pointed it is, you changed from nsDependentCString to nsCString. So, up to you for other lines because you don't need to touch them for fixing this bug. Perhaps, we should fix it in another bug if there are a lot of points which should be replaced with nsAuto*String.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c7efa29d66
Upgrade to Hunspell 1.5.0. r=masayuki

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25c7efa29d66

Comment 10

[Tyson Smith [:tsmith]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Tyson, have you been fuzzing Hunspell lately?

Not since August. I can likely add it to the continuous fuzzing system this quarter but I want to make sure we are targeting the correct APIs.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I decided to back this out for causing bug 1320746 and also because it causes some API compatibility issues for systems that use the system Hunspell. Those have been resolved upstream, so I'll try to land the future 1.5.x release that includes the Hungarian fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9444e271b391686b1238a20c20175b8bd7d7b400

Comment 12

[Carsten Book [:Tomcat]]

backout also from central
https://hg.mozilla.org/mozilla-central/rev/9444e271b391

Comment 13

[Dimitrij Mijoski]

What is the status  for updating to Hunspell version 1.5.4?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Haven't had time to work on it yet. Will try as soon as I can.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

2016-11-30 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.4
	* Fixes the command COMPOUNDSYLLABLE used in Hungarian dictionary.

2016-11-28 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.3
	* Removed a #include from hunspell.hxx that was creating trouble

2016-11-27 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.2
	* Reverted full backward compatibility with 1.4 public API, again

2016-11-27 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.1
	* Reverted full backward compatibility with 1.4 public API

2016-11-26 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5
	* Fixed the changelog notice bellow about the API. I had a mistake.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Attachment: Update Hunspell to version 1.5.4

Looks good on Try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86cf532c4a6f7db4896cd2ca8a4fe349843c6f0e

avada, can you please test a build from this Try push to confirm that everything works as expected for you?
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-86cf532c4a6f7db4896cd2ca8a4fe349843c6f0e/

Masayuki-san, did you want to take another look at this before I re-push it (assuming avada's testing goes well)? No changes on the Gecko side compared to the 1.5.0 patch you previously reviewed.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Masayuki-san, did you want to take another look at this before I re-push it
> (assuming avada's testing goes well)? No changes on the Gecko side compared
> to the 1.5.0 patch you previously reviewed.

No. Up to you.

Comment 18

[avada]

Comment on attachment 8817444 [details] [diff] [review]
Update Hunspell to version 1.5.4

Review of attachment 8817444 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. No breakages in spellcheck with the hungarian dictionary.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33044cd4c5db
Upgrade to Hunspell 1.5.4. r=masayuki

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/33044cd4c5db

Comment 21

[Mike Hommey [:glandium]]

This requires a version check to be added to pkg_check_modules in moz.configure, because this breaks building with system version 1.4.

Comment 22

[Landry Breuil (:gaston)]

Seconded, i had a build failure w/ 53.0b1 using --with-system-hunspell - our system version is 1.3.2... (yeah ancient i know)